Address gx-arch-review feedback: tighten DI, remove god-object reach

Findings from the architectural review on the four-commit follow-up:

* Protocol/filter parity (high): ``_handle_authorization`` was reading
  ``context.trans.app.config.is_admin_user`` directly and only handled
  ``tool_type == "data_manager"`` — re-introduced the god-object chain
  the Protocol was meant to keep out, and silently dropped polymorphism
  for any other admin-only Tool subclass. ``ToolFilterContext`` now
  declares ``allow_user_access(user, attempting_access)``; ``Tool``
  satisfies it natively, and a new ``_IndexEntryFilterAdapter`` wraps a
  ``ToolIndexEntry`` with an injected admin-check callable. Filters call
  ``tool.allow_user_access(user)`` polymorphically — no
  ``context.trans.app.config`` reach.

* ``ToolSearchTuning`` dataclass (medium): ``ToolWhooshIndex`` used to
  take the full ``GalaxyAppConfiguration`` but only read ~10 boost /
  ngram knobs. New frozen dataclass holds those keys; the search module
  no longer imports ``GalaxyAppConfiguration`` (even under
  ``TYPE_CHECKING``). The lazy toolbox builds a ``ToolSearchTuning`` at
  construction via ``ToolSearchTuning.from_config``.

* No more internal construction in ``_get_search_index`` (medium): the
  Whoosh import is hoisted to the module top, the cached
  ``_whoosh_search_index`` / ``_whoosh_search_index_version`` are
  declared on ``LazyToolBox.__init__`` (no more ``getattr`` defensive
  reads), and ``_get_search_index`` now has a proper return annotation.

* Drop defensive ``getattr`` (low): ``self.app.config.use_lazy_toolbox``
  and ``self.app.config.tool_search_index_dir`` are typed config fields,
  read them directly.

* Code-structure nits: ``import os`` and ``import shutil`` are hoisted
  to the top of ``search.py``; ``ToolFilterContext.labels`` and
  ``tags`` are typed ``list[str]``; the inline
  ``from galaxy.tool_source_store.search import ToolWhooshIndex`` is
  gone.

* Tests: ``mock_tool`` regains an ``allow_user_access`` callable so the
  ``Bunch`` mock keeps satisfying the Protocol. No coverage loss.

Out of scope: the controller-business-logic refactor flagged in
``api/tool_dependencies.py:toolbox_install/uninstall`` is pre-PR and
belongs in a separate change.

lib/galaxy/tool_source_store/search.py

@@ -11,7 +11,10 @@ on-disk Whoosh index and queries it with ``BM25F`` scoring.
 """
 
 import logging
+import os
 import re
+import shutil
+from dataclasses import dataclass
 from typing import (
     Optional,
     TYPE_CHECKING,
@@ -38,7 +41,6 @@ from whoosh.writing import AsyncWriter
 from galaxy.util import unicodify
 
 if TYPE_CHECKING:
-    from galaxy.config import GalaxyAppConfiguration
     from .index import (
         ToolIndex,
         ToolIndexEntry,
@@ -47,39 +49,78 @@ if TYPE_CHECKING:
 log = logging.getLogger(__name__)
 
 
-def build_schema(config: "GalaxyAppConfiguration") -> Schema:
+@dataclass(frozen=True)
+class ToolSearchTuning:
+    """Whoosh schema/boost knobs for tool search.
+
+    Mirrors the ``tool_*`` keys on ``GalaxyAppConfiguration`` so this module
+    can build a search index without importing the full Galaxy config type
+    or carrying a god-object reference. Wire one in at the call site —
+    typically via :meth:`from_config`.
+    """
+
+    id_boost: float
+    name_boost: float
+    name_exact_multiplier: float
+    stub_boost: float
+    section_boost: float
+    description_boost: float
+    label_boost: float
+    ngram_minsize: int
+    ngram_maxsize: int
+    enable_ngram_search: bool
+    ngram_factor: float
+
+    @classmethod
+    def from_config(cls, config) -> "ToolSearchTuning":
+        return cls(
+            id_boost=float(config.tool_id_boost),
+            name_boost=float(config.tool_name_boost),
+            name_exact_multiplier=float(config.tool_name_exact_multiplier),
+            stub_boost=float(config.tool_stub_boost),
+            section_boost=float(config.tool_section_boost),
+            description_boost=float(config.tool_description_boost),
+            label_boost=float(config.tool_label_boost),
+            ngram_minsize=int(config.tool_ngram_minsize),
+            ngram_maxsize=int(config.tool_ngram_maxsize),
+            enable_ngram_search=bool(config.tool_enable_ngram_search),
+            ngram_factor=float(config.tool_ngram_factor),
+        )
+
+
+def build_schema(tuning: ToolSearchTuning) -> Schema:
     """Whoosh schema mirroring ``ToolPanelViewSearch`` field set + boosts."""
     schema_conf: dict = {
         "id": ID(stored=True, unique=True),
         "id_exact": NGRAMWORDS(
-            minsize=config.tool_ngram_minsize,
-            maxsize=config.tool_ngram_maxsize,
-            field_boost=(config.tool_id_boost * config.tool_name_exact_multiplier),
+            minsize=tuning.ngram_minsize,
+            maxsize=tuning.ngram_maxsize,
+            field_boost=(tuning.id_boost * tuning.name_exact_multiplier),
         ),
         "name_exact": TEXT(
-            field_boost=(config.tool_name_boost * config.tool_name_exact_multiplier),
+            field_boost=(tuning.name_boost * tuning.name_exact_multiplier),
             analyzer=analysis.IDTokenizer() | analysis.LowercaseFilter(),
         ),
-        "stub": KEYWORD(field_boost=float(config.tool_stub_boost)),
-        "section": TEXT(field_boost=float(config.tool_section_boost)),
-        "edam_operations": TEXT(field_boost=float(config.tool_section_boost)),
-        "edam_topics": TEXT(field_boost=float(config.tool_section_boost)),
-        "repository": TEXT(field_boost=float(config.tool_section_boost)),
-        "owner": TEXT(field_boost=float(config.tool_section_boost)),
+        "stub": KEYWORD(field_boost=tuning.stub_boost),
+        "section": TEXT(field_boost=tuning.section_boost),
+        "edam_operations": TEXT(field_boost=tuning.section_boost),
+        "edam_topics": TEXT(field_boost=tuning.section_boost),
+        "repository": TEXT(field_boost=tuning.section_boost),
+        "owner": TEXT(field_boost=tuning.section_boost),
         "description": TEXT(
-            field_boost=config.tool_description_boost,
+            field_boost=tuning.description_boost,
             analyzer=analysis.StemmingAnalyzer(),
         ),
-        "labels": KEYWORD(field_boost=float(config.tool_label_boost)),
+        "labels": KEYWORD(field_boost=tuning.label_boost),
     }
-    if config.tool_enable_ngram_search:
+    if tuning.enable_ngram_search:
         schema_conf["name"] = NGRAMWORDS(
-            minsize=config.tool_ngram_minsize,
-            maxsize=config.tool_ngram_maxsize,
-            field_boost=(float(config.tool_name_boost) * config.tool_ngram_factor),
+            minsize=tuning.ngram_minsize,
+            maxsize=tuning.ngram_maxsize,
+            field_boost=tuning.name_boost * tuning.ngram_factor,
         )
     else:
-        schema_conf["name"] = TEXT(field_boost=float(config.tool_name_boost))
+        schema_conf["name"] = TEXT(field_boost=tuning.name_boost)
     return Schema(**schema_conf)
 
 
@@ -139,14 +180,12 @@ class ToolWhooshIndex:
     no Galaxy-specific encoding, so an operator can reopen it offline.
     """
 
-    def __init__(self, index_dir: str, config: "GalaxyAppConfiguration") -> None:
+    def __init__(self, index_dir: str, tuning: ToolSearchTuning) -> None:
         self.index_dir = index_dir
-        self.config = config
-        self.schema = build_schema(config)
+        self.tuning = tuning
+        self.schema = build_schema(tuning)
 
     def _open(self) -> index.FileIndex:
-        import os
-
         os.makedirs(self.index_dir, exist_ok=True)
         if index.exists_in(self.index_dir):
             ix = index.open_dir(self.index_dir)
@@ -156,8 +195,6 @@ class ToolWhooshIndex:
             # poison ranking.
             log.info("ToolWhooshIndex schema changed; rebuilding %s", self.index_dir)
             ix.close()
-            import shutil
-
             shutil.rmtree(self.index_dir)
             os.makedirs(self.index_dir, exist_ok=True)
         return index.create_in(self.index_dir, schema=self.schema)
@@ -197,8 +234,6 @@ class ToolWhooshIndex:
         """
         if not query or not query.strip():
             return []
-        import os
-
         if not (os.path.isdir(self.index_dir) and index.exists_in(self.index_dir)):
             return []
         ix = index.open_dir(self.index_dir)

lib/galaxy/tools/__init__.py

@@ -689,10 +689,10 @@ class ToolBox(AbstractToolBox):
             ToolSource if found in store, None otherwise.
         """
         # Default (eager) deployments parse from disk; only consult the store
-        # when the operator opted into ``use_lazy_toolbox``. The macro-id and
-        # silent-wrong-source defects (see LAZY_TOOLBOX_FOLLOWUPS.md) live on
-        # the path that walks the store on every call.
-        if not getattr(self.app.config, "use_lazy_toolbox", False):
+        # when the operator opted into ``use_lazy_toolbox``. The silent-wrong-source
+        # defect that used to live on this path only manifested under the lazy
+        # toolbox, so default deployments stay on the eager parse path.
+        if not self.app.config.use_lazy_toolbox:
             return None
 
         store = getattr(self.app, "tool_source_store", None)

lib/galaxy/tools/lazy_toolbox.py

@@ -34,6 +34,10 @@ from galaxy.tool_source_store.index import (
     ToolIndex,
     ToolIndexEntry,
 )
+from galaxy.tool_source_store.search import (
+    ToolSearchTuning,
+    ToolWhooshIndex,
+)
 from galaxy.tool_util.id_util import (
     extract_short_id_from_guid,
     extract_tool_id_from_file,
@@ -301,6 +305,34 @@ class _LazyToolsByIdView:
         return dict(self.items())
 
 
+class _IndexEntryFilterAdapter:
+    """Wraps a :class:`ToolIndexEntry` so it satisfies ``ToolFilterContext``.
+
+    ``Tool.allow_user_access`` consults ``self.app`` to do per-subclass admin
+    checks (``DataManagerTool`` is admin-only). ``ToolIndexEntry`` is a lightweight
+    record with no app reference; instead of dragging one onto every entry,
+    the adapter takes the admin-check callable at construction time. Filter
+    functions call ``tool.allow_user_access(user)`` polymorphically and never
+    reach through ``context.trans.app.config``.
+    """
+
+    __slots__ = ("entry", "_is_admin_user")
+
+    def __init__(self, entry: "ToolIndexEntry", is_admin_user):
+        self.entry = entry
+        self._is_admin_user = is_admin_user
+
+    def __getattr__(self, name):
+        return getattr(self.entry, name)
+
+    def allow_user_access(self, user, attempting_access: bool = True) -> bool:
+        if self.entry.require_login and user is None:
+            return False
+        if self.entry.tool_type == "data_manager":
+            return user is not None and bool(self._is_admin_user(user))
+        return True
+
+
 class LazyToolBox(ToolBox):
     """
     ToolBox that loads tools on-demand from the tool source store.
@@ -335,6 +367,10 @@ class LazyToolBox(ToolBox):
         self._tool_object_cache: LRUCache = LRUCache(maxsize=cache_size)
         self._cache_lock = threading.RLock()
         self._reload_count = 0
+        # Whoosh search infrastructure — built lazily on first ``search_tools``
+        # call and re-built whenever the underlying ``ToolIndex`` version changes.
+        self._whoosh_search_index: Optional[ToolWhooshIndex] = None
+        self._whoosh_search_index_version: Optional[str] = None
 
         # Initialize core attributes that AbstractToolBox.__init__ would set
         # We do this manually to avoid loading all tools
@@ -2297,29 +2333,23 @@ class LazyToolBox(ToolBox):
             return self._tool_index.get(tool_id)
         return None
 
-    def _search_index_dir(self) -> Optional[str]:
-        """Resolve the directory the lazy Whoosh search index lives in.
+    def _get_search_index(self) -> Optional[ToolWhooshIndex]:
+        """Return the cached :class:`ToolWhooshIndex` or build one on demand.
 
-        Sits under ``tool_search_index_dir`` (the same root the eager
-        ``ToolBoxSearch`` uses) in a sub-folder so neither path stomps the
-        other.
+        Resolves the index path under ``tool_search_index_dir`` (the same
+        root the eager ``ToolBoxSearch`` uses), in a sub-folder so neither
+        path stomps the other. ``ToolSearchTuning`` is built from
+        ``self.app.config`` once and passed in — the search index code
+        doesn't need a god-object reference.
         """
-        base = getattr(self.app.config, "tool_search_index_dir", None)
+        if self._whoosh_search_index is not None:
+            return self._whoosh_search_index
+        base = self.app.config.tool_search_index_dir
         if not base:
             return None
-        return os.path.join(base, "_lazy_default")
-
-    def _get_search_index(self):
-        """Return the cached :class:`ToolWhooshIndex` or build one on demand."""
-        existing = getattr(self, "_whoosh_search_index", None)
-        if existing is not None:
-            return existing
-        index_dir = self._search_index_dir()
-        if not index_dir:
-            return None
-        from galaxy.tool_source_store.search import ToolWhooshIndex
-
-        self._whoosh_search_index = ToolWhooshIndex(index_dir=index_dir, config=self.app.config)
+        index_dir = os.path.join(base, "_lazy_default")
+        tuning = ToolSearchTuning.from_config(self.app.config)
+        self._whoosh_search_index = ToolWhooshIndex(index_dir=index_dir, tuning=tuning)
         return self._whoosh_search_index
 
     def search_tools(self, query: str, limit: int = 50) -> list[ToolIndexEntry]:
@@ -2336,8 +2366,7 @@ class LazyToolBox(ToolBox):
         if searcher is None:
             return self._tool_index.search(query, limit=limit)
         current_version = self._tool_index.compute_version()
-        last_version = getattr(self, "_whoosh_search_index_version", None)
-        if last_version != current_version:
+        if self._whoosh_search_index_version != current_version:
             try:
                 searcher.build(self._tool_index)
                 self._whoosh_search_index_version = current_version
@@ -2397,14 +2426,15 @@ class LazyToolBox(ToolBox):
             return super().to_dict(trans, in_panel=True, tool_help=tool_help, view=view, **kwds)
 
         filter_method = self._build_filter_method(trans)
+        # The adapter exposes ``allow_user_access`` so the stock
+        # ``_handle_authorization`` filter is polymorphic across
+        # ``Tool`` (eager) and the index view (lazy) — see
+        # ``_IndexEntryFilterAdapter``.
+        is_admin_user = self.app.config.is_admin_user
         rval = []
         for _tool_id, entry in self._tool_index.entries.items():
-            # ``filter_method`` honours ``_not_hidden`` + ``_handle_authorization``
-            # (the always-on stock filters) plus any ``tool_filters`` /
-            # ``user_tool_filters`` the operator configured. Both stock filters
-            # read only fields on ``ToolFilterContext``, which ``ToolIndexEntry``
-            # exposes — no Tool materialisation needed.
-            if not filter_method(entry, panel_item_types.TOOL):
+            candidate = _IndexEntryFilterAdapter(entry, is_admin_user)
+            if not filter_method(candidate, panel_item_types.TOOL):
                 continue
             rval.append(self._index_entry_to_api_dict(entry))