From d75a51872c6cf7055445f097c1183602d793922a Mon Sep 17 00:00:00 2001 From: Ofer LaOr Date: Fri, 18 Sep 2026 23:39:09 -0700 Subject: [PATCH] fix: delegated child never leases a same-provider pool entry for another endpoint A delegated child with an explicit base_url (e.g. an Azure OpenAI resource under provider "openai") shared the parent's "openai" credential pool on bare provider equality, and _lease_child_credential bound whatever entry acquire_lease() picked. _swap_credential adopts the entry's base_url as well, so the child was silently rebound to https://api.openai.com/v1, sent the Azure key there, got HTTP 401 and only then fell back (#68237). _resolve_child_credential_pool now requires endpoint coherence on top of provider identity (credential_pool_matches_provider + at least one entry whose base_url matches the child's) for both the shared parent pool and the provider's loaded pool; a mismatched pool is not attached and the child keeps its fixed credential. _lease_child_credential validates the leased entry against the child's base_url and, on a mixed pool, releases the wrong-host lease and leases an endpoint-matching entry by id instead. Entries and adapters without endpoint metadata cannot rebind and are accepted unchanged. Salvaged from #68240 (@oferlaor); the acquire_lease/leased_entry filter parameters and the extra pool-level helper were reduced to the two small delegate-side predicates. --- contributors/emails/ofer@openclaw.ai | 1 + tests/tools/test_delegate.py | 40 ++++++++++++++++++++++++++++ tools/delegate_tool_child_run.py | 13 ++++++++- tools/delegate_tool_config.py | 34 +++++++++++++++++++++-- 4 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 contributors/emails/ofer@openclaw.ai diff --git a/contributors/emails/ofer@openclaw.ai b/contributors/emails/ofer@openclaw.ai new file mode 100644 index 0000000000..88f8a4a0cd --- /dev/null +++ b/contributors/emails/ofer@openclaw.ai @@ -0,0 +1 @@ +oferlaor diff --git a/tests/tools/test_delegate.py b/tests/tools/test_delegate.py index 9dacf1ad3d..ec9cee514d 100644 --- a/tests/tools/test_delegate.py +++ b/tests/tools/test_delegate.py @@ -1281,6 +1281,26 @@ class TestChildCredentialPoolResolution(unittest.TestCase): result = _resolve_child_credential_pool("openrouter", parent) self.assertIs(result, mock_pool) + def test_same_provider_pool_for_another_endpoint_is_not_shared(self): + """#68237: an Azure child must not lease the parent's public-OpenAI ``openai`` pool — the lease swaps the + child's base_url too, sending the pooled key to the wrong host. A pool with an entry for the child's endpoint + is still shared.""" + from agent.credential_pool import CredentialPool, PooledCredential + + azure = "https://res.cognitiveservices.azure.com/openai/v1" + def _pool(url): + return CredentialPool("openai", [PooledCredential( + provider="openai", id=url, label=url, auth_type="api_key", priority=0, source="env:X", + access_token="k", base_url=url)]) + parent = _make_mock_parent() + parent.provider, parent.base_url = "openai", azure + + parent._credential_pool = _pool("https://api.openai.com/v1") + with patch("tools.delegate_tool_config._loaded_pool", return_value=None): + self.assertIsNone(_resolve_child_credential_pool("openai", parent, azure)) + parent._credential_pool = _pool(azure) + self.assertIs(_resolve_child_credential_pool("openai", parent, azure), parent._credential_pool) + # --- Custom-endpoint identity resolution (issue #7833) --- @@ -1363,6 +1383,26 @@ class TestChildCredentialLeasing(unittest.TestCase): self.assertEqual(result["status"], "error") child._credential_pool.release_lease.assert_called_once_with("cred-a") + def test_lease_binds_only_an_entry_for_the_child_endpoint(self): + """#68237: on a mixed same-provider pool the least-leased pick may target another host; the child must end up + bound to the entry for its own base_url, with the wrong-host lease released.""" + from agent.credential_pool import CredentialPool, PooledCredential + from tools.delegate_tool_child_run import _lease_child_credential + + azure = "https://res.cognitiveservices.azure.com/openai/v1" + def _entry(eid, url): + return PooledCredential(provider="openai", id=eid, label=eid, auth_type="api_key", priority=0, + source=f"env:{eid}", access_token=f"key-{eid}", base_url=url) + pool = CredentialPool("openai", [_entry("pub", "https://api.openai.com/v1"), _entry("az", azure)]) + pool.acquire_lease("az") # tilt least-leased selection toward the public entry + child = MagicMock(provider="openai", base_url=azure, _credential_pool=pool) + + _pool, lease_id = _lease_child_credential(child) + + self.assertEqual(lease_id, "az") + self.assertEqual(child._swap_credential.call_args[0][0].base_url, azure) + self.assertEqual(pool._active_leases, {"az": 2}) + class TestDelegateHeartbeat(unittest.TestCase): """Heartbeat propagates child activity to parent during delegation. diff --git a/tools/delegate_tool_child_run.py b/tools/delegate_tool_child_run.py index a2b7f72a61..a24c0d9b3a 100644 --- a/tools/delegate_tool_child_run.py +++ b/tools/delegate_tool_child_run.py @@ -390,14 +390,25 @@ def _defer_close_after_timeout(child: Any, child_future: Any) -> None: _resweep_timer.start() def _lease_child_credential(child: Any) -> tuple[Any, Optional[str]]: - """Lease a credential from the child's pool (if any) and bind it; ``(pool, lease_id)``.""" + """Lease a credential from the child's pool (if any) and bind it; ``(pool, lease_id)``. The bound entry must + serve the child's endpoint: on a mixed same-provider pool the least-leased pick may target another host, so it is + released and an endpoint-matching entry is leased by id instead (#68237).""" child_pool = getattr(child, "_credential_pool", None) if child_pool is None: return None, None + from tools.delegate_tool_config import _entry_serves_endpoint + base_url = getattr(child, "base_url", None) leased_cred_id = child_pool.acquire_lease() if leased_cred_id is not None: with _quiet("Failed to bind child to leased credential: %s"): leased_entry = child_pool.current() + if not _entry_serves_endpoint(leased_entry, base_url): + child_pool.release_lease(leased_cred_id) + leased_entry = next( + (e for e in child_pool.entries() if e.last_status != "dead" and _entry_serves_endpoint(e, base_url)), + None, + ) + leased_cred_id = child_pool.acquire_lease(leased_entry.id) if leased_entry is not None else None if leased_entry is not None and hasattr(child, "_swap_credential"): child._swap_credential(leased_entry) return child_pool, leased_cred_id diff --git a/tools/delegate_tool_config.py b/tools/delegate_tool_config.py index e8c399a2e6..1c6b8ae1e9 100644 --- a/tools/delegate_tool_config.py +++ b/tools/delegate_tool_config.py @@ -214,6 +214,30 @@ def _loaded_pool(key: Any): pool = load_pool(key) return pool if pool is not None and pool.has_credentials() else None +def _entry_serves_endpoint(entry: Any, base_url: Any) -> bool: + """Whether a pooled credential may be bound to a child running at ``base_url``. ``_swap_credential`` adopts the + entry's base_url too, so a same-provider entry for another endpoint (public OpenAI vs. an Azure resource) would + send the child's request — and the entry's key — to the wrong host (#68237). Entries or children without + endpoint metadata (legacy adapters, test doubles) cannot rebind and are accepted.""" + if not isinstance(base_url, str) or not base_url: + return True + entry_url = getattr(entry, "runtime_base_url", None) or getattr(entry, "base_url", None) + if not isinstance(entry_url, str) or not entry_url: + return True + from hermes_cli.route_identity import normalize_route_base_url + return normalize_route_base_url(entry_url) == normalize_route_base_url(base_url) + +def _pool_serves_endpoint(pool: Any, provider: Optional[str], base_url: Optional[str]) -> bool: + """Provider identity AND at least one entry for the child's endpoint; pools without entry metadata pass.""" + from agent.credential_pool import credential_pool_matches_provider + if not credential_pool_matches_provider(pool, provider, base_url=base_url): + return False + entries_fn = getattr(pool, "entries", None) + if not callable(entries_fn): + return True + entries = entries_fn() + return not isinstance(entries, list) or any(_entry_serves_endpoint(entry, base_url) for entry in entries) + def _resolve_child_credential_pool( effective_provider: Optional[str], parent_agent, effective_base_url: Optional[str] = None, ): @@ -244,8 +268,14 @@ def _resolve_child_credential_pool( return parent_pool return _loaded_pool(child_key) if parent_pool is not None and effective_provider == parent_provider: - return parent_pool - return _loaded_pool(effective_provider) + if not effective_base_url or _pool_serves_endpoint(parent_pool, effective_provider, effective_base_url): + return parent_pool + logger.debug("Parent %s pool has no entry for child endpoint %s; not sharing it", + effective_provider, effective_base_url) + pool = _loaded_pool(effective_provider) + if pool is not None and effective_base_url and not _pool_serves_endpoint(pool, effective_provider, effective_base_url): + return None # child keeps its fixed credential + return pool except Exception as exc: if effective_provider == "custom": logger.debug("Could not resolve custom credential pool for child endpoint '%s': %s", effective_base_url, exc)