fix: work around SAP GUI COM error 618 on collection.Item(index) (mcp#804) (#61)

* fix: work around SAP GUI COM error 618 on collection.Item(index)

On some Windows/SAP GUI hosts, calling `<GuiComponentCollection>.Item(<int>)`
under pywin32 late binding raises com_error 618 "Bad index type for collection
access." even when the collection is healthy — Children.Count == 1 but
Children.Item(0) throws, while Children(0) (default member) and
Children.ElementAt(0) return the element fine. The index marshalling differs
per host; identical code works elsewhere.

This silently broke desktop login: wait_for_session reads conn.children[0],
GuiComponentCollection.__getitem__ called Item(index), the com_error was
swallowed by a broad except, and login spun to a 30s "No session available"
timeout without ever entering credentials (Hochfrequenz/sapgui.mcp#804).

Fix: centralise collection indexing in a new `com_collection_item(collection,
index)` helper that tries Item(index) first (unchanged on working hosts — can
only add success cases), then falls back to ElementAt(index), then the default
member, and re-raises the original Item error if all fail. Route every
integer-indexed COM collection access through it (GuiComponentCollection,
GuiCollection, application connection cleanup, combobox entries, BDT probe,
dump_tree). Also log the previously-swallowed poll error in wait_for_session at
debug so a future occurrence is diagnosable instead of an opaque timeout.

Fallbacks (ElementAt / default member) are empirically confirmed on the
affected host to return the GuiSession that Item rejects.

Tests: com_collection_item fallback order + re-raise semantics; collection
__getitem__/iteration recovery; and an end-to-end wait_for_session regression
guard driving the real GuiConnection path with a raising Item.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(_wrap): address Copilot review on com_collection_item

- Preserve the original Item() traceback: use a bare `raise` instead of
  `raise item_error` (which reset the traceback to the re-raise line). Verified
  the bare raise re-raises the original Item error, not a fallback's, and keeps
  the Item frame in the traceback.
- Fix docstring line-wrap that split "byte-for-byte" across lines.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: hf-kklein <konstantin.klein+claude@hochfrequenz.de>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 3 people

src/sapsucker/_wrap.py

@@ -59,3 +59,54 @@ def wrap_com_object(com_obj: Any) -> GuiComponent:
             extra={"type_as_number": type_num, "com_type": getattr(com_obj, "Type", "?")},
         )
     return cls(com_obj)  # type: ignore[misc, no-any-return]
+
+
+def com_collection_item(com_collection: Any, index: int) -> Any:
+    """Return the raw COM element at *index* from a SAP GUI COM collection.
+
+    Works around SAP GUI COM error 618, ``"Bad index type for collection
+    access."``, which some hosts raise for ``collection.Item(<int>)`` under
+    pywin32 late binding **even when the collection is perfectly healthy**.
+
+    Observed on a fresh Windows 11 / SAP GUI 8.0 host (Hochfrequenz/sapgui.mcp#804):
+    ``connection.Children.Count == 1``, yet ``Children.Item(0)`` raises 618 while
+    ``Children(0)`` (the collection's default member) and ``Children.ElementAt(0)``
+    both return the element fine. The same code works on other hosts, so the
+    difference is in how the integer index is marshalled to ``Item`` — not the
+    collection, the element, or the server. This silently broke ``login()``:
+    ``wait_for_session`` reads ``conn.children[0]``, the ``Item`` call raised, and a
+    broad ``except`` swallowed it, so login spun to a 30s timeout with no session.
+
+    Strategy, in order (first that succeeds wins):
+      1. ``Item(index)`` — the historical path. Tried first so behaviour is
+         byte-for-byte identical on the (majority of) hosts where it already
+         works; this fix can only *add* success cases, never remove one.
+      2. ``ElementAt(index)`` — SAP's documented integer accessor; accepted on
+         hosts that reject ``Item``'s marshalled index.
+      3. default member ``collection(index)`` — last resort; empirically accepted
+         wherever ``ElementAt`` is.
+
+    If every strategy fails, the ORIGINAL ``Item`` error is re-raised, so a
+    genuinely bad index still surfaces its real COM error rather than a masked one.
+    """
+    try:
+        return com_collection.Item(index)
+    except Exception:  # pylint: disable=broad-exception-caught
+        # ``getattr(..., None)`` returns a live method proxy for late-bound
+        # CDispatch objects (so this branch is reached on real SAP hosts), and
+        # ``None`` for plain objects that genuinely lack the method.
+        element_at = getattr(com_collection, "ElementAt", None)
+        if element_at is not None:
+            try:
+                return element_at(index)
+            except Exception:  # pylint: disable=broad-exception-caught
+                pass
+        try:
+            return com_collection(index)  # default member: collection(index)
+        except Exception:  # pylint: disable=broad-exception-caught
+            pass
+        # Bare ``raise`` (not ``raise <saved exc>``) so the ORIGINAL traceback from
+        # the failed ``Item(index)`` call is preserved. Once the inner fallback
+        # handlers have completed, the exception being handled here is again the
+        # original ``Item`` error, so this re-raises it — not a fallback's error.
+        raise

src/sapsucker/components/application.py

@@ -6,7 +6,7 @@
 
 from typing import TYPE_CHECKING, Any
 
-from sapsucker._wrap import wrap_com_object
+from sapsucker._wrap import com_collection_item, wrap_com_object
 from sapsucker.components.base import GuiContainer
 
 if TYPE_CHECKING:
@@ -116,7 +116,7 @@ def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None:
             # so closing from the end avoids skipping connections.
             for i in range(self._com.Children.Count - 1, -1, -1):
                 try:
-                    self._com.Children.Item(i).CloseConnection()
+                    com_collection_item(self._com.Children, i).CloseConnection()
                 except Exception:  # noqa: BLE001
                     pass
         except Exception:  # noqa: BLE001

src/sapsucker/components/base.py

@@ -10,7 +10,7 @@
 from typing import TYPE_CHECKING, Any
 
 from sapsucker._errors import ElementNotFoundError
-from sapsucker._wrap import wrap_com_object
+from sapsucker._wrap import com_collection_item, wrap_com_object
 
 if TYPE_CHECKING:
     from sapsucker.components.collection import GuiComponentCollection
@@ -275,7 +275,7 @@ def _probe_bdt_fields(com_obj: Any) -> list[ElementInfo]:
         try:
             found = com_obj.FindAllByNameEx("*", type_num)
             for j in range(found.Count):
-                child = found.Item(j)
+                child = com_collection_item(found, j)
                 child_id = str(_safe_com_attr(child, "Id", ""))
                 if child_id in seen_ids:
                     continue
@@ -506,7 +506,7 @@ def _dump_tree_recursive(com_obj: Any, depth: int, max_depth: int) -> list[Eleme
     if count > 0:
         for i in range(count):
             try:
-                child = children_com.Item(i)
+                child = com_collection_item(children_com, i)
             except Exception:
                 continue
             is_container = bool(_safe_com_attr(child, "ContainerType", False))

src/sapsucker/components/collection.py

@@ -4,7 +4,7 @@
 
 from typing import Any, Iterator
 
-from sapsucker._wrap import wrap_com_object
+from sapsucker._wrap import com_collection_item, wrap_com_object
 from sapsucker.components.base import GuiComponent
 
 __all__ = ["GuiCollection", "GuiComponentCollection"]
@@ -27,7 +27,7 @@ def __getitem__(self, index: int) -> GuiComponent:
             index += length
         if index < 0 or index >= length:
             raise IndexError(f"Index {index} out of range for collection of length {length}")
-        return wrap_com_object(self._com.Item(index))
+        return wrap_com_object(com_collection_item(self._com, index))
 
     def __iter__(self) -> Iterator[GuiComponent]:
         """Iterate over all wrapped components."""
@@ -55,12 +55,12 @@ def __getitem__(self, index: int) -> Any:
             index += length
         if index < 0 or index >= length:
             raise IndexError(f"Index {index} out of range for collection of length {length}")
-        return self._com.Item(index)
+        return com_collection_item(self._com, index)
 
     def __iter__(self) -> Iterator[Any]:
         """Iterate over all items."""
         for i in range(self._com.Count):
-            yield self._com.Item(i)
+            yield com_collection_item(self._com, i)
 
     def __repr__(self) -> str:
         return f"GuiCollection(count={self._com.Count})"

src/sapsucker/components/combobox.py

@@ -4,6 +4,7 @@
 
 from typing import Any
 
+from sapsucker._wrap import com_collection_item
 from sapsucker.components.base import GuiVComponent
 
 __all__ = ["GuiComboBox", "GuiComboBoxEntry"]
@@ -54,7 +55,7 @@ def entries(self) -> list[GuiComboBoxEntry]:
         """Return all entries as a list of GuiComboBoxEntry."""
         result = []
         for i in range(self._com.Entries.Count):
-            result.append(GuiComboBoxEntry(self._com.Entries.Item(i)))
+            result.append(GuiComboBoxEntry(com_collection_item(self._com.Entries, i)))
         return result
 
     @property

src/sapsucker/login.py

@@ -336,7 +336,12 @@ def wait_for_session(conn: Any, timeout: int = 30) -> GuiSession:
                 if isinstance(session, GuiSessionCls):
                     return session
         except Exception:
-            pass
+            # Tolerate transient errors while the session is still materialising,
+            # but log them: a persistent error here (e.g. the SAP GUI COM error 618
+            # "Bad index type" that ``conn.children[0]`` used to raise on some hosts,
+            # sapgui.mcp#804) would otherwise silently spin to the timeout below with
+            # no clue as to why. See ``com_collection_item`` for that specific fix.
+            logger.debug("wait_for_session_poll_error", exc_info=True)
         time.sleep(0.5)
     raise SapGuiTimeoutError(f"No session available on connection after {timeout}s")
 

unittests/test_collection.py

@@ -71,6 +71,27 @@ def test_empty(self):
         assert len(gcc) == 0
         assert list(gcc) == []
 
+    def test_getitem_falls_back_when_item_raises_bad_index(self):
+        """Indexing still works when raw Item(i) raises SAP GUI error 618.
+
+        Regression guard for sapgui.mcp#804: on some hosts ``Item(<int>)`` raises
+        ``com_error`` 618 "Bad index type for collection access." while
+        ``ElementAt(<int>)`` returns the element fine. com_collection_item must
+        transparently fall back so ``collection[i]`` (and, through it, iteration)
+        keep working.
+        """
+        items = _make_component_items(2)
+        col = MagicMock()
+        col.Count = 2
+        col.Item.side_effect = RuntimeError("Bad index type for collection access.")
+        col.ElementAt = lambda i: items[i]
+
+        gcc = GuiComponentCollection(col)
+        assert gcc[0].com is items[0]
+        assert gcc[1].com is items[1]
+        # Iteration routes through __getitem__, so it recovers too.
+        assert [r.com for r in gcc] == items
+
 
 class TestGuiCollection:
     def test_len(self):

unittests/test_factory.py

@@ -9,6 +9,7 @@
     _TYPE_MAP,
     wrap_com_object,
 )
+from sapsucker._wrap import com_collection_item
 from sapsucker.components.base import GuiComponent
 from sapsucker.components.button import GuiButton
 from sapsucker.components.field import GuiTextField
@@ -94,3 +95,98 @@ def test_shell_subtype_entry(self, sub_type, cls):
         com = make_mock_com(type_as_number=122, type_name="GuiShell", SubType=sub_type)
         result = wrap_com_object(com)
         assert isinstance(result, cls)
+
+
+class _FakeComError(Exception):
+    """Stand-in for pywintypes.com_error carrying SAP GUI error 618.
+
+    Mirrors the real payload observed on the failing host (sapgui.mcp#804):
+    ``(-2147352567, 'Exception occurred.', (618, 'saplogon',
+    'Bad index type for collection access.', None, 0, 0), None)``.
+    """
+
+
+_BAD_INDEX_ARGS = (
+    -2147352567,
+    "Exception occurred.",
+    (618, "saplogon", "Bad index type for collection access.", None, 0, 0),
+    None,
+)
+
+
+class _FakeCollection:
+    """Fake SAP GUI COM collection with independently-toggleable accessors.
+
+    Models the exact quirk from sapgui.mcp#804: ``Item(<int>)`` can raise COM
+    error 618 while ``ElementAt(<int>)`` and the default member ``collection(<int>)``
+    return the element fine.
+    """
+
+    def __init__(self, items, *, item_ok=True, element_at_ok=True, default_ok=True, has_element_at=True):
+        self._items = items
+        self._item_ok = item_ok
+        self._element_at_ok = element_at_ok
+        self._default_ok = default_ok
+        self._has_element_at = has_element_at
+        self.Count = len(items)
+
+    def Item(self, index):
+        if not self._item_ok:
+            raise _FakeComError(*_BAD_INDEX_ARGS)
+        return self._items[index]
+
+    def __call__(self, index):  # default member: collection(index)
+        if not self._default_ok:
+            raise _FakeComError(*_BAD_INDEX_ARGS)
+        return self._items[index]
+
+    def __getattr__(self, name):
+        # Only synthesise ElementAt; everything else is a genuine miss so that
+        # ``getattr(col, "ElementAt", None)`` returns None when absent.
+        if name == "ElementAt" and self.__dict__.get("_has_element_at", False):
+
+            def _element_at(index):
+                if not self._element_at_ok:
+                    raise _FakeComError(*_BAD_INDEX_ARGS)
+                return self._items[index]
+
+            return _element_at
+        raise AttributeError(name)
+
+
+class TestComCollectionItem:
+    """Tests for com_collection_item() — the SAP GUI error 618 workaround (mcp#804)."""
+
+    def test_uses_item_when_it_works(self):
+        """When Item(index) succeeds it is used and ElementAt is never consulted."""
+        items = ["a", "b", "c"]
+        # ElementAt would raise if reached — proves it is not reached.
+        col = _FakeCollection(items, item_ok=True, element_at_ok=False)
+        assert com_collection_item(col, 1) == "b"
+
+    def test_falls_back_to_element_at_on_bad_index(self):
+        """Item raising 618 falls back to ElementAt(index)."""
+        items = ["a", "b", "c"]
+        col = _FakeCollection(items, item_ok=False, element_at_ok=True)
+        assert com_collection_item(col, 2) == "c"
+
+    def test_falls_back_to_default_member_when_item_and_element_at_fail(self):
+        """Item and ElementAt both raising falls back to the default member call."""
+        items = ["a", "b", "c"]
+        col = _FakeCollection(items, item_ok=False, element_at_ok=False, default_ok=True)
+        assert com_collection_item(col, 0) == "a"
+
+    def test_falls_back_to_default_member_when_element_at_absent(self):
+        """A collection without ElementAt falls straight to the default member."""
+        items = ["a", "b"]
+        col = _FakeCollection(items, item_ok=False, has_element_at=False, default_ok=True)
+        assert com_collection_item(col, 1) == "b"
+
+    def test_reraises_original_item_error_when_all_strategies_fail(self):
+        """If every access strategy fails, the ORIGINAL Item error is re-raised."""
+        col = _FakeCollection(["a"], item_ok=False, element_at_ok=False, default_ok=False)
+        with pytest.raises(_FakeComError) as exc_info:
+            com_collection_item(col, 0)
+        # The re-raised error is the one from Item (carries the 618 payload), not a
+        # masked/replaced exception from a later fallback.
+        assert exc_info.value.args[2][0] == 618

unittests/test_login.py

@@ -597,3 +597,30 @@ def children(self):
 
         with pytest.raises(SapGuiTimeoutError, match="No session available"):
             wait_for_session(_RaisingConn(), timeout=1)
+
+    def test_returns_session_when_raw_children_item_raises_bad_index(self):
+        """End-to-end regression guard for sapgui.mcp#804.
+
+        On the failing host ``connection.Children.Count == 1`` but the raw
+        ``Children.Item(0)`` raises SAP GUI error 618 "Bad index type for
+        collection access.", which the old poll loop swallowed and spun to a
+        timeout. Driving the REAL GuiConnection -> GuiComponentCollection ->
+        com_collection_item path, wait_for_session must now recover via the
+        ElementAt fallback and return the session.
+        """
+        from sapsucker.components.connection import GuiConnection
+        from sapsucker.components.session import GuiSession as GuiSessionCls
+        from unittests.conftest import make_mock_com
+
+        session_com = make_mock_com(type_as_number=12, type_name="GuiSession", name="ses[0]")
+        children = MagicMock()
+        children.Count = 1
+        children.Item.side_effect = RuntimeError("Bad index type for collection access.")
+        children.ElementAt = lambda i: session_com
+        conn_com = MagicMock()
+        conn_com.DisabledByServer = False
+        conn_com.Children = children
+
+        result = wait_for_session(GuiConnection(conn_com), timeout=2)
+        assert isinstance(result, GuiSessionCls)
+        assert result.com is session_com