feat(connectors): let team members edit a team-shared Custom API

src/xagent/web/api/mcp.py

@@ -2001,7 +2001,18 @@ def _global_config_tampered(server_data: MCPServerUpdate, server: MCPServer) ->
 class _TeamOwnedUserMCP:
     """Stand-in for a missing UserMCPServer row: a team connector the user does
     not personally own. Exposes the attributes the response builders read with
-    not-owned defaults (usable, but not editable/deletable)."""
+    not-owned defaults (usable, but not editable/deletable).
+
+    ``__slots__`` declares only ``user_id`` as a real per-instance attribute.
+    Every other name below is a class attribute, not a slot, so assigning to
+    it on an instance -- ``stand_in.is_active = False``, say -- raises
+    ``AttributeError`` instead of silently creating a shadowing instance
+    attribute the caller's own row never backs. A caller admitted through
+    this stand-in has no association row to write, so nothing here should
+    ever be writable.
+    """
+
+    __slots__ = ("user_id",)
 
     is_owner = False
     can_edit = False
@@ -2016,7 +2027,15 @@ def __init__(self, user_id: int) -> None:
 
 
 class _TeamOwnedUserApi:
-    """Stand-in for a missing UserCustomApi row (team-owned, not user-owned)."""
+    """Stand-in for a missing UserCustomApi row (team-owned, not user-owned).
+
+    Same reasoning as ``_TeamOwnedUserMCP`` above: ``__slots__`` leaves
+    ``user_id`` as the only attribute an instance can hold, so a write to
+    ``can_edit``, ``is_active`` or ``is_default`` raises ``AttributeError``
+    rather than shadowing the class default with a value nothing persists.
+    """
+
+    __slots__ = ("user_id",)
 
     can_edit = False
     is_active = True

src/xagent/web/services/connector_team_scope.py

@@ -47,10 +47,12 @@
 | Call site | Caller holds while the hook runs | Committed before asking | ``caller_holds_lock`` |
 | --- | --- | --- | --- |
 | ``custom_api.update_custom_api`` | the ``custom_apis`` definition row, ``FOR UPDATE``, on the payloads that write that row | no | ``True`` |
+| ``custom_api._recheck_team_access_under_definition_lock`` | the ``custom_apis`` definition row, ``FOR UPDATE``, taken by ``update_custom_api`` before this call | no | ``True`` |
 | ``custom_api.delete_custom_api`` | the ``custom_apis`` definition row, ``FOR UPDATE`` | no | ``True`` |
 | ``mcp.update_mcp_server`` | the ``mcp_servers`` definition row, ``FOR UPDATE ... KEY SHARE``, on the payloads that write that row | no | ``True`` |
 | ``mcp.teardown_mcp_app_server`` | three row locks: ``public_mcp_apps``, ``mcp_servers``, ``user_mcpservers`` | no, within this function -- see the note below | ``True`` |
 | ``mcp.delete_mcp_server`` | two row locks: ``mcp_servers`` and ``user_mcpservers``, taken by ``_lock_active_mcp_oauth_lifecycle`` before this call | no | ``True`` |
+| ``custom_api._resolve_custom_api_for_request`` | nothing -- this resolution runs before either of its two routes takes any lock | no | ``False`` |
 
 ``mcp.teardown_mcp_app_server`` is a helper, not a route: it has no route
 decorator and no caller in this repository outside tests. "Nothing committed
@@ -69,11 +71,11 @@
 The remaining slots declare nothing. Every ``visibility`` and
 ``team_visibility`` call site is lock-free, and one of the ``team_visibility``
 paths runs on a lazily created session that may not be in a transaction at
-all. ``access`` has no call site in this repository; a caller that adds one
-while holding a lock owes this table a row and owes the call
-``caller_holds_lock=True``. One shape must never declare it: a call site that
-has already committed its own work before asking, because refusing there
-reports a failure for an operation that fully succeeded.
+all. A caller that adds a new ``access`` call site while holding a lock owes
+this table a row and owes the call ``caller_holds_lock=True``. One shape must
+never declare it: a call site that has already committed its own work before
+asking, because refusing there reports a failure for an operation that fully
+succeeded.
 
 What the check is not
 ---------------------

tests/web/api/test_custom_api.py

@@ -1,4 +1,5 @@
 import ast
+import importlib
 import inspect
 from datetime import datetime
 from types import SimpleNamespace
@@ -29,6 +30,94 @@
     set_connector_team_hooks,
 )
 
+_SEAM_MODULE = "xagent.web.api.custom_api"
+
+# Every top-level function in this module that can reach an installed
+# connector team hook. Written out so the discovery below cannot pass by
+# finding nothing.
+_SEAM_REACHING_FUNCTIONS = {
+    "_resolve_custom_api_for_request",
+    "_recheck_team_access_under_definition_lock",
+    "get_custom_api",
+    "update_custom_api",
+    "delete_custom_api",
+}
+
+
+def _functions_reaching_the_connector_seam() -> dict[str, ast.AST]:
+    """Every top-level function in this module that can reach an installed
+    connector team hook.
+
+    Seeded on the functions that import ``connector_team_scope`` in their own
+    body, which is how every call site in this module reaches the seam, then
+    closed transitively over plain-name calls, because one route reaches it
+    only through a helper (``get_custom_api`` through
+    ``_resolve_custom_api_for_request``). A seed-only check would miss exactly
+    the route this test exists for.
+    """
+    module = importlib.import_module(_SEAM_MODULE)
+    tree = ast.parse(inspect.getsource(module))
+    functions = {
+        node.name: node
+        for node in tree.body
+        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
+    }
+    reaching = {
+        name
+        for name, node in functions.items()
+        if any(
+            isinstance(child, ast.ImportFrom)
+            and child.module is not None
+            and child.module.endswith("connector_team_scope")
+            for child in ast.walk(node)
+        )
+    }
+    changed = True
+    while changed:
+        changed = False
+        for name, node in functions.items():
+            if name in reaching:
+                continue
+            called = {
+                child.func.id
+                for child in ast.walk(node)
+                if isinstance(child, ast.Call) and isinstance(child.func, ast.Name)
+            }
+            if called & reaching:
+                reaching.add(name)
+                changed = True
+    return {name: functions[name] for name in reaching}
+
+
+def test_the_discovery_of_seam_reaching_functions_is_not_vacuous():
+    """Pins the enumeration itself, so the assertion below cannot pass by
+    finding nothing."""
+    assert set(_functions_reaching_the_connector_seam()) == _SEAM_REACHING_FUNCTIONS
+
+
+def test_no_function_that_reaches_the_connector_seam_is_a_coroutine():
+    """An installed connector team hook may be slow -- the seam is designed on
+    the assumption that the installing application answers from its own
+    tables. FastAPI runs a coroutine route on the event loop thread itself, so
+    a slow hook call inside an ``async def`` stalls every other request the
+    process is serving, not just this one; a plain ``def`` goes to the
+    threadpool instead, where a slow call occupies one worker.
+
+    Enumerated by reachability rather than by a hand-written list of routes:
+    an earlier fix for this same risk class swept siblings along the "takes a
+    row lock" axis and therefore missed a route that calls a hook without
+    taking one.
+    """
+    offenders = [
+        name
+        for name, node in _functions_reaching_the_connector_seam().items()
+        if isinstance(node, ast.AsyncFunctionDef)
+    ]
+    assert offenders == [], (
+        "these functions can reach an installed connector team hook while "
+        f"running on the event loop thread: {sorted(offenders)}"
+    )
+
 
 def test_custom_api_models_env_validation():
     # Valid creation
@@ -244,8 +333,7 @@ async def test_create_custom_api_rejects_runtime_static_header_conflict():
     assert "Invalid runtime configuration" in str(exc_info.value.detail)
 
 
-@pytest.mark.asyncio
-async def test_get_custom_api():
+def test_get_custom_api():
     db = MagicMock(spec=Session)
     user = User(id=1)
 
@@ -262,19 +350,18 @@ async def test_get_custom_api():
 
     db.query().filter().first.return_value = mock_user_api
 
-    res = await get_custom_api(10, current_user=user, db=db)
+    res = get_custom_api(10, current_user=user, db=db)
     assert res.id == 10
     assert res.name == "test_api"
 
 
-@pytest.mark.asyncio
-async def test_get_custom_api_not_found():
+def test_get_custom_api_not_found():
     db = MagicMock(spec=Session)
     user = User(id=1)
     db.query().filter().first.return_value = None
 
     with pytest.raises(HTTPException) as exc_info:
-        await get_custom_api(99, current_user=user, db=db)
+        get_custom_api(99, current_user=user, db=db)
     assert exc_info.value.status_code == 404