Merge pull request #5313 from RaresKeY/fix/chat-web-search-explicit-deny

fix(chat): require explicit web search enable

Author: bitboody

routes/chat_routes.py

@@ -42,7 +42,12 @@
     _enforce_chat_privileges,
 )
 from src.action_intents import ToolIntent, classify_tool_intent as _classify_tool_intent
-from src.tool_policy import build_effective_tool_policy
+from src.tool_policy import (
+    WEB_TOOL_NAMES,
+    build_effective_tool_policy,
+    is_web_search_explicitly_denied,
+    web_search_enabled_for_turn,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -583,10 +588,7 @@ async def chat_stream(request: Request) -> StreamingResponse:
         # below). Skill extraction should only learn from real agent sessions,
         # not chats we quietly promoted for a notes/calendar intent.
         user_requested_agent = (chat_mode == "agent")
-        _search_enabled = (
-            str(allow_web_search).lower() == "true"
-            or str(use_web).lower() == "true"
-        )
+        _search_enabled = web_search_enabled_for_turn(allow_web_search, use_web)
         # Intent auto-escalation: if the user is clearly asking the assistant
         # to create a todo, reminder, or calendar event, promote chat → agent
         # for this turn so the LLM has access to manage_notes / manage_calendar.
@@ -870,24 +872,20 @@ async def chat_stream(request: Request) -> StreamingResponse:
 
         # Build disabled-tools set from frontend toggles + user privileges
         disabled_tools = set()
-        # Only disable bash/web_search when the caller *explicitly* set them
-        # to a falsy value.  When unset (None), defer to per-user privilege
-        # checks below — this lets admins with can_use_bash=True use bash
-        # by default without having to send allow_bash in every request.
+        # Only disable bash when the caller *explicitly* set it to a falsy
+        # value. When unset (None), defer to per-user privilege checks below.
+        # Web search is per-turn opt-in: either the chat pre-search setting
+        # (`use_web=true`) or agent web toggle (`allow_web_search=true`) must
+        # explicitly enable it.
         if allow_bash is not None and str(allow_bash).lower() != "true":
             disabled_tools.add("bash")
         _explicit_web_intent = bool(_tool_intent and _tool_intent.category == "web")
-        if (
-            allow_web_search is not None
-            and str(allow_web_search).lower() != "true"
-        ):
-            disabled_tools.add("web_search")
-            disabled_tools.add("web_fetch")
+        if is_web_search_explicitly_denied(allow_web_search) or not _search_enabled:
+            disabled_tools.update(WEB_TOOL_NAMES)
         if _explicit_web_intent:
             # A direct lookup/search request should not drift into personal
-            # tools or shell fallbacks. We still keep web_search/web_fetch
-            # available even when the frontend toggle is stale/falsy because
-            # the user's words are the stronger signal.
+            # tools or shell fallbacks. It can only use web_search/web_fetch
+            # when the request's explicit web setting enabled them.
             disabled_tools.update({
                 "bash", "python",
                 "search_chats", "manage_skills", "manage_memory",
@@ -897,11 +895,12 @@ async def chat_stream(request: Request) -> StreamingResponse:
                 "manage_notes", "manage_calendar", "manage_tasks",
                 "api_call", "builtin_browser",
             })
-            disabled_tools.discard("web_search")
-            disabled_tools.discard("web_fetch")
+            if _search_enabled:
+                disabled_tools.difference_update(WEB_TOOL_NAMES)
+            else:
+                disabled_tools.update(WEB_TOOL_NAMES)
         elif _search_enabled:
-            disabled_tools.discard("web_search")
-            disabled_tools.discard("web_fetch")
+            disabled_tools.difference_update(WEB_TOOL_NAMES)
 
         # Nobody/incognito mode: deny tools that would expose the user's
         # persistent memory, past chats, or other identity-linked data.
@@ -952,14 +951,7 @@ async def chat_stream(request: Request) -> StreamingResponse:
         from src.settings import get_setting
         _global_disabled = get_setting("disabled_tools", [])
         if _global_disabled and isinstance(_global_disabled, list):
-            explicit_web_allowed = (
-                _explicit_web_intent
-                or (allow_web_search is not None and str(allow_web_search).lower() == "true")
-            )
-            if explicit_web_allowed:
-                disabled_tools.update(t for t in _global_disabled if t not in {"web_search", "web_fetch"})
-            else:
-                disabled_tools.update(_global_disabled)
+            disabled_tools.update(_global_disabled)
 
         # Light auto-escalation: the user is in chat mode and just expressed a
         # notes/calendar/email intent. Grant the relevant managers but withhold
@@ -1411,10 +1403,8 @@ def _on_research_done(_sid, _result, _sources, _findings):
                     _max_rounds = max(1, min(_max_rounds, 200))
 
                     _forced_tools = None
-                    if _explicit_web_intent:
-                        _forced_tools = {"web_search", "web_fetch"}
-                    elif _search_enabled:
-                        _forced_tools = {"web_search", "web_fetch"}
+                    if _search_enabled:
+                        _forced_tools = set(WEB_TOOL_NAMES)
 
                     async for chunk in stream_agent_loop(
                         sess.endpoint_url,

src/agent_loop.py

@@ -24,7 +24,7 @@
 from src.settings import get_setting
 from src.prompt_security import untrusted_context_message
 from src.tool_security import blocked_tools_for_owner, plan_mode_disabled_tools
-from src.tool_policy import GUIDE_ONLY_DIRECTIVE, ToolPolicy
+from src.tool_policy import GUIDE_ONLY_DIRECTIVE, WEB_TOOL_NAMES, ToolPolicy
 from src.tool_utils import _truncate, get_mcp_manager
 from src.agent_tools import (
     parse_tool_blocks,
@@ -321,7 +321,7 @@ def _load_mcp_disabled_map() -> Dict[str, set]:
 }
 
 _DOMAIN_TOOL_MAP = {
-    "web": {"web_search", "web_fetch"},
+    "web": set(WEB_TOOL_NAMES),
     "documents": {"create_document", "edit_document", "update_document", "suggest_document", "manage_documents"},
     "email": {"list_email_accounts", "list_emails", "read_email", "send_email", "reply_to_email", "bulk_email", "archive_email", "delete_email", "mark_email_read", "resolve_contact", "manage_contact"},
     "cookbook": {"download_model", "serve_model", "serve_preset", "list_serve_presets", "list_served_models", "stop_served_model", "tail_serve_output", "list_downloads", "cancel_download", "search_hf_models", "list_cached_models", "list_cookbook_servers", "adopt_served_model"},
@@ -2847,13 +2847,12 @@ async def stream_agent_loop(
         if "email" in (_intent.get("domains") or set()):
             _relevant_tools.add("ui_control")
         if "web" in (_intent.get("domains") or set()):
-            _relevant_tools.update({"web_search", "web_fetch"})
-            _removed_web_blocks = sorted({"web_search", "web_fetch"} & disabled_tools)
-            if _removed_web_blocks:
-                disabled_tools.difference_update({"web_search", "web_fetch"})
+            _relevant_tools.update(WEB_TOOL_NAMES)
+            _blocked_web_tools = sorted(WEB_TOOL_NAMES & disabled_tools)
+            if _blocked_web_tools:
                 logger.info(
-                    "[agent-intent] web turn forced search tools enabled; removed disabled=%s",
-                    _removed_web_blocks,
+                    "[agent-intent] web domain selected but search tools remain disabled=%s",
+                    _blocked_web_tools,
                 )
         if "ui" in (_intent.get("domains") or set()):
             _relevant_tools.add("ui_control")
@@ -2887,9 +2886,9 @@ async def stream_agent_loop(
             _relevant_tools = set(ALWAYS_AVAILABLE)
         _relevant_tools.update({"read_file", "grep", "ls", "manage_documents"})
 
-    # Per-request forced tools are stronger than retrieval. Search toggles and
-    # explicit lookup turns must make web tools visible even when tool RAG
-    # misses them; route-level disabled_tools decides what else is allowed.
+    # Per-request forced tools are stronger than retrieval. Explicit search
+    # settings make web tools visible even when tool RAG misses them;
+    # route-level disabled_tools decides what remains allowed.
     if not guide_only and forced_tools:
         forced_set = {t for t in forced_tools if t not in disabled_tools}
         if _relevant_tools is None:

src/tool_policy.py

@@ -16,6 +16,39 @@
     "output they will produce locally."
 )
 
+WEB_TOOL_NAMES = frozenset({"web_search", "web_fetch"})
+
+
+def tool_toggle_enabled(value: object) -> bool:
+    """Return true only for explicit true-like tool toggle values."""
+
+    return str(value).lower() == "true"
+
+
+def tool_toggle_explicitly_denied(value: object) -> bool:
+    """Return true when a caller explicitly supplied a non-true toggle value."""
+
+    return value is not None and not tool_toggle_enabled(value)
+
+
+def is_web_search_explicitly_denied(allow_web_search: object) -> bool:
+    """Whether the web-search agent toggle was explicitly set to false."""
+
+    return tool_toggle_explicitly_denied(allow_web_search)
+
+
+def web_search_enabled_for_turn(allow_web_search: object, use_web: object = None) -> bool:
+    """Return true only when this request explicitly enables web search.
+
+    Agent mode sends ``allow_web_search``; chat-mode pre-search sends
+    ``use_web``. If both are present, an explicit ``allow_web_search=false``
+    wins so a stale or conflicting intent path cannot re-enable web tools.
+    """
+
+    if is_web_search_explicitly_denied(allow_web_search):
+        return False
+    return tool_toggle_enabled(allow_web_search) or tool_toggle_enabled(use_web)
+
 
 _COMMON_TOOL_NAMES = {
     "api_call",

tests/test_chat_route_tool_policy.py

@@ -1,12 +1,11 @@
-"""Issue #3229 — allow_bash / allow_web_search must work for JSON API callers
-and admin users must get bash enabled by default.
+"""Issue #3229 and explicit web-toggle regressions.
 
 Bug: allow_bash and allow_web_search were only read from form_data, so JSON
 API callers (Content-Type: application/json) always had bash disabled.
 
 Fix: (1) Read from JSON body as fallback.
-     (2) Only add bash/web_search to disabled_tools when explicitly set to a
-         falsy value; when unset (None), defer to per-user privilege checks.
+     (2) Keep bash on the privilege fallback when unset.
+     (3) Require an explicit per-turn web setting before exposing web tools.
 """
 
 import ast
@@ -15,6 +14,11 @@
 import pytest
 
 from src.action_intents import classify_tool_intent
+from src.tool_policy import (
+    WEB_TOOL_NAMES,
+    is_web_search_explicitly_denied,
+    web_search_enabled_for_turn,
+)
 
 _CHAT_ROUTES = Path(__file__).resolve().parent.parent / "routes" / "chat_routes.py"
 
@@ -76,8 +80,7 @@ def test_allow_web_search_reads_from_body_as_fallback():
 
 
 def test_disabled_tools_respects_missing_vs_explicit_toggles():
-    """When allow_bash is not set (None), bash must NOT be unconditionally
-    added to disabled_tools.  The per-user privilege check handles it.
+    """Bash still defers to privileges, but web is an explicit per-turn opt-in.
     """
     source = _CHAT_ROUTES.read_text(encoding="utf-8")
 
@@ -88,11 +91,14 @@ def test_disabled_tools_respects_missing_vs_explicit_toggles():
     assert "allow_bash is not None" in source, (
         "disabled_tools check must guard against allow_bash being None"
     )
-    assert "allow_web_search is not None" in source, (
-        "disabled_tools check must guard against allow_web_search being None"
+    assert "web_search_enabled_for_turn(allow_web_search, use_web)" in source, (
+        "web tools must be gated through the explicit per-turn web setting"
+    )
+    assert "disabled_tools.update(WEB_TOOL_NAMES)" in source, (
+        "disabled_tools must add web_search/web_fetch when web is not explicitly enabled"
     )
-    assert "and not _explicit_web_intent" not in source, (
-        "explicit allow_web_search=false must not be overridden by prompt web intent"
+    assert "_forced_tools = set(WEB_TOOL_NAMES)" in source, (
+        "web tools should only be forced visible from the explicit web setting"
     )
 
 
@@ -102,31 +108,48 @@ def test_disabled_tools_respects_missing_vs_explicit_toggles():
 def _build_disabled_tools(
     allow_bash=None,
     allow_web_search=None,
+    use_web=None,
     can_use_bash=True,
     can_use_browser=True,
     explicit_web_intent=False,
+    global_disabled=None,
 ):
     """Replicate the disabled-tools logic from chat_stream for unit testing.
 
     Returns the set of tool names that would be disabled.
     """
     disabled_tools = set()
 
-    # Issue #3229 fix: only disable when explicitly set to a falsy value.
+    # Issue #3229 fix: only disable bash when explicitly set to a falsy value.
     if allow_bash is not None and str(allow_bash).lower() != "true":
         disabled_tools.add("bash")
-    if (
-        allow_web_search is not None
-        and str(allow_web_search).lower() != "true"
-    ):
-        disabled_tools.add("web_search")
-        disabled_tools.add("web_fetch")
+    search_enabled = web_search_enabled_for_turn(allow_web_search, use_web)
+    if is_web_search_explicitly_denied(allow_web_search) or not search_enabled:
+        disabled_tools.update(WEB_TOOL_NAMES)
+    if explicit_web_intent:
+        disabled_tools.update({
+            "bash", "python",
+            "search_chats", "manage_skills", "manage_memory",
+            "read_file", "write_file", "edit_file",
+            "create_document", "edit_document", "update_document",
+            "send_email", "reply_to_email",
+            "manage_notes", "manage_calendar", "manage_tasks",
+            "api_call", "builtin_browser",
+        })
+        if search_enabled:
+            disabled_tools.difference_update(WEB_TOOL_NAMES)
+        else:
+            disabled_tools.update(WEB_TOOL_NAMES)
+    elif search_enabled:
+        disabled_tools.difference_update(WEB_TOOL_NAMES)
 
     # Enforce per-user privileges
     if not can_use_bash:
         disabled_tools.update({"bash", "python", "read_file", "write_file"})
     if not can_use_browser:
         disabled_tools.add("builtin_browser")
+    if global_disabled and isinstance(global_disabled, list):
+        disabled_tools.update(global_disabled)
 
     return disabled_tools
 
@@ -157,6 +180,20 @@ def test_json_body_allow_web_search_false_disables_web():
     assert "web_fetch" in disabled
 
 
+def test_chat_mode_use_web_true_enables_web():
+    """Chat pre-search sends use_web=true as the explicit web setting."""
+    disabled = _build_disabled_tools(use_web="true")
+    assert "web_search" not in disabled
+    assert "web_fetch" not in disabled
+
+
+def test_allow_web_search_false_wins_over_use_web_true():
+    """The agent web toggle hard-denies web even if another path says use_web=true."""
+    disabled = _build_disabled_tools(allow_web_search="false", use_web="true")
+    assert "web_search" in disabled
+    assert "web_fetch" in disabled
+
+
 @pytest.mark.parametrize(
     "message",
     [
@@ -180,6 +217,21 @@ def test_explicit_false_disables_web_despite_prompt_web_intent(message):
     assert "web_fetch" in disabled
 
 
+def test_prompt_web_intent_does_not_enable_web_without_setting():
+    """Prompt-derived web intent alone must not expose web tools."""
+    intent = classify_tool_intent("look up the latest docs")
+    assert intent is not None
+    assert intent.category == "web"
+
+    disabled = _build_disabled_tools(
+        allow_web_search=None,
+        use_web=None,
+        explicit_web_intent=True,
+    )
+    assert "web_search" in disabled
+    assert "web_fetch" in disabled
+
+
 def test_admin_user_gets_bash_enabled_by_default():
     """When allow_bash is not set and user has can_use_bash privilege,
     bash must NOT be disabled.
@@ -188,13 +240,11 @@ def test_admin_user_gets_bash_enabled_by_default():
     assert "bash" not in disabled
 
 
-def test_admin_user_gets_web_search_enabled_by_default():
-    """When allow_web_search is not set and user has normal privileges,
-    web_search must NOT be disabled.
-    """
+def test_web_search_disabled_by_default_without_explicit_turn_setting():
+    """Missing web settings must not expose web tools by default."""
     disabled = _build_disabled_tools(allow_web_search=None)
-    assert "web_search" not in disabled
-    assert "web_fetch" not in disabled
+    assert "web_search" in disabled
+    assert "web_fetch" in disabled
 
 
 def test_non_privileged_user_without_explicit_flag_still_disabled():
@@ -213,6 +263,16 @@ def test_non_privileged_user_explicit_true_overridden_by_privilege():
     assert "bash" in disabled
 
 
+def test_global_disabled_web_wins_over_explicit_web_enable():
+    """Admin-level disabled tools are still a hard deny."""
+    disabled = _build_disabled_tools(
+        allow_web_search="true",
+        global_disabled=["web_search", "web_fetch"],
+    )
+    assert "web_search" in disabled
+    assert "web_fetch" in disabled
+
+
 def test_form_data_none_body_true_works():
     """Simulates: form_data has no allow_bash, body has allow_bash=true.
     After the fallback (`form_data.get(...) or body.get(...)`), allow_bash