Address greptile: decouple the client timeouts from EnvironmentVariables

Greptile flagged that adding the four timeout fields to the shared
EnvironmentVariables model meant a malformed value broke far more than the
client factory: refresh() is called from ~20 unguarded places, including
AgentWorker startup and EnvAuth.auth_flow on every request. The try/except in
create_async_agentex_client() did not contain that, it only made it look
handled.

Removing the try/except alone made it worse. agentex/lib/adk/utils/__init__.py
constructs TemplatingModule() at module scope, which builds a client, so
importing the ADK started requiring AGENT_NAME and ACP_URL to be set. The
suppressed exception had been hiding that.

Read the four values from os.environ in client.py instead. The shared model is
untouched, so startup, auth and import are unaffected, and a malformed value
raises a ValueError naming the variable at the point it is used rather than
being swallowed. Adds a regression test asserting the timeout does not depend
on the shared model.

Author: chakrris

src/agentex/lib/adk/utils/_modules/client.py

@@ -1,3 +1,4 @@
+import os
 from typing import override
 
 import httpx
@@ -26,21 +27,40 @@ def auth_flow(self, request):
         yield request
 
 
+# HTTP timeouts for the AgentEx client, in seconds. Defaults match the SDK's
+# DEFAULT_TIMEOUT, so leaving these unset changes nothing.
+_TIMEOUT_ENV_DEFAULTS = {
+    "connect": ("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", 5.0),
+    "read": ("AGENTEX_CLIENT_READ_TIMEOUT_SECONDS", 300.0),
+    "write": ("AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS", 300.0),
+    "pool": ("AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS", 300.0),
+}
+
+
 def _timeout_from_env() -> httpx.Timeout:
     """Build the client timeout from environment variables.
 
-    Defaults match the SDK's DEFAULT_TIMEOUT, so an unconfigured process behaves
-    exactly as before. The connect timeout is the one worth raising: an AgentEx
-    backend accepts connections serially, so connect latency grows with the number
-    of callers and the 5s default is reached when a few hundred are in flight.
+    Read from ``os.environ`` rather than from ``EnvironmentVariables``. That model
+    is loaded by worker startup and by ``EnvAuth.auth_flow`` on every request, and
+    ``agentex.lib.adk.utils`` builds a client at import time, so a field added
+    there would make a malformed timeout break all three. Reading here keeps the
+    blast radius to the one value that is actually wrong.
+
+    The connect timeout is the one worth raising: an AgentEx backend accepts
+    connections serially, so connect latency grows with the number of callers and
+    the 5s default is reached when a few hundred are in flight.
     """
-    env_vars = EnvironmentVariables.refresh()
-    return httpx.Timeout(
-        connect=env_vars.AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS,
-        read=env_vars.AGENTEX_CLIENT_READ_TIMEOUT_SECONDS,
-        write=env_vars.AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS,
-        pool=env_vars.AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS,
-    )
+    values = {}
+    for field, (env_var, default) in _TIMEOUT_ENV_DEFAULTS.items():
+        raw = os.environ.get(env_var)
+        if raw is None or raw.strip() == "":
+            values[field] = default
+            continue
+        try:
+            values[field] = float(raw)
+        except ValueError as exc:
+            raise ValueError(f"{env_var} must be a number in seconds, got {raw!r}") from exc
+    return httpx.Timeout(**values)
 
 
 def create_async_agentex_client(**kwargs) -> AsyncAgentex:
@@ -50,11 +70,7 @@ def create_async_agentex_client(**kwargs) -> AsyncAgentex:
     AGENTEX_CLIENT_*_TIMEOUT_SECONDS environment variables.
     """
     if "timeout" not in kwargs:
-        try:
-            kwargs["timeout"] = _timeout_from_env()
-        except Exception as exc:
-            # Never let timeout configuration stop a client being created.
-            logger.warning("Falling back to SDK default timeout: %r", exc)
+        kwargs["timeout"] = _timeout_from_env()
     client = AsyncAgentex(**kwargs)
     client._client.auth = EnvAuth()
     return client

src/agentex/lib/environment_variables.py

@@ -20,16 +20,12 @@ class EnvVarKeys(str, Enum):
     TEMPORAL_ADDRESS = "TEMPORAL_ADDRESS"
     REDIS_URL = "REDIS_URL"
     AGENTEX_BASE_URL = "AGENTEX_BASE_URL"
-    # AgentEx client HTTP timeouts (seconds)
-    AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS = "AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS"
-    AGENTEX_CLIENT_READ_TIMEOUT_SECONDS = "AGENTEX_CLIENT_READ_TIMEOUT_SECONDS"
-    AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS = "AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS"
-    AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS = "AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS"
     # Agent Identifiers
     AGENT_NAME = "AGENT_NAME"
     AGENT_DESCRIPTION = "AGENT_DESCRIPTION"
     AGENT_ID = "AGENT_ID"
     AGENT_VERSION = "AGENT_VERSION"
+    AGENT_COMMIT_SHA = "AGENT_COMMIT_SHA"
     AGENT_API_KEY = "AGENT_API_KEY"
     # ACP Configuration
     ACP_URL = "ACP_URL"
@@ -66,20 +62,18 @@ class EnvironmentVariables(BaseModel):
     TEMPORAL_ADDRESS: str | None = "localhost:7233"
     REDIS_URL: str | None = None
     AGENTEX_BASE_URL: str | None = "http://localhost:5003"
-    # HTTP timeouts for the AgentEx client, in seconds. Defaults match the
-    # SDK's DEFAULT_TIMEOUT, so leaving these unset changes nothing.
-    # Raise the connect timeout when many concurrent activities share one
-    # backend: accepts queue, and 5s is reached at a few hundred in flight.
-    AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS: float = 5.0
-    AGENTEX_CLIENT_READ_TIMEOUT_SECONDS: float = 300.0
-    AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS: float = 300.0
-    AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS: float = 300.0
     # Agent Identifiers
     AGENT_NAME: str
     AGENT_DESCRIPTION: str | None = None
     AGENT_ID: str | None = None
     # Build/version discriminator (image tag or git sha), set by the deployment
     AGENT_VERSION: str | None = None
+    # The agent's source commit, baked into the image or set by the deployment.
+    # Unlike AGENT_VERSION this is expected to be a git SHA and nothing else, and
+    # it is OPT-IN: nothing is stamped unless the agent calls
+    # `adk.code_revision.enable()`, which also refuses a value that is not a git
+    # object name. See agentex.lib.core.tracing.code_revision.
+    AGENT_COMMIT_SHA: str | None = None
     AGENT_API_KEY: str | None = None
     ACP_TYPE: str | None = "async"
     AGENT_INPUT_TYPE: str | None = None

tests/lib/test_client_timeout_env.py

@@ -13,32 +13,14 @@
 import httpx
 import pytest
 
-import agentex.lib.environment_variables as env_module
 from agentex.lib.adk.utils._modules.client import (
     _timeout_from_env,
     create_async_agentex_client,
 )
 
 
-@pytest.fixture(autouse=True)
-def _clear_env_cache():
-    """EnvironmentVariables.refresh() memoises into a module global."""
-    env_module.refreshed_environment_variables = None
-    yield
-    env_module.refreshed_environment_variables = None
-
-
-def _set_env(monkeypatch, **overrides: str) -> None:
-    # EnvironmentVariables has required fields; set them so construction succeeds.
-    monkeypatch.setenv("AGENT_NAME", "test-agent")
-    monkeypatch.setenv("ACP_URL", "http://localhost:8000")
-    for key, value in overrides.items():
-        monkeypatch.setenv(key, value)
-
-
-def test_defaults_match_the_sdk_default_timeout(monkeypatch):
+def test_defaults_match_the_sdk_default_timeout():
     """An unconfigured process must behave exactly as it did before."""
-    _set_env(monkeypatch)
     timeout = _timeout_from_env()
     assert timeout.connect == 5.0
     assert timeout.read == 300.0
@@ -47,21 +29,18 @@ def test_defaults_match_the_sdk_default_timeout(monkeypatch):
 
 
 def test_connect_timeout_is_configurable(monkeypatch):
-    _set_env(monkeypatch, AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS="30")
+    monkeypatch.setenv("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", "30")
     timeout = _timeout_from_env()
     assert timeout.connect == 30.0
     # the others are untouched
     assert timeout.read == 300.0
 
 
 def test_all_four_are_configurable(monkeypatch):
-    _set_env(
-        monkeypatch,
-        AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS="30",
-        AGENTEX_CLIENT_READ_TIMEOUT_SECONDS="120",
-        AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS="90",
-        AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS="60",
-    )
+    monkeypatch.setenv("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", "30")
+    monkeypatch.setenv("AGENTEX_CLIENT_READ_TIMEOUT_SECONDS", "120")
+    monkeypatch.setenv("AGENTEX_CLIENT_WRITE_TIMEOUT_SECONDS", "90")
+    monkeypatch.setenv("AGENTEX_CLIENT_POOL_TIMEOUT_SECONDS", "60")
     timeout = _timeout_from_env()
     assert (timeout.connect, timeout.read, timeout.write, timeout.pool) == (
         30.0,
@@ -71,14 +50,21 @@ def test_all_four_are_configurable(monkeypatch):
     )
 
 
+def test_an_empty_value_falls_back_to_the_default():
+    """An unset variable and one set to the empty string mean the same thing."""
+    with pytest.MonkeyPatch.context() as mp:
+        mp.setenv("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", "")
+        assert _timeout_from_env().connect == 5.0
+
+
 def test_client_picks_up_the_env_timeout(monkeypatch):
-    _set_env(monkeypatch, AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS="30")
+    monkeypatch.setenv("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", "30")
     client = create_async_agentex_client(api_key="test", base_url="http://localhost:5003")
     assert client.timeout.connect == 30.0
 
 
 def test_explicit_timeout_wins_over_the_environment(monkeypatch):
-    _set_env(monkeypatch, AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS="30")
+    monkeypatch.setenv("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", "30")
     client = create_async_agentex_client(
         api_key="test",
         base_url="http://localhost:5003",
@@ -87,15 +73,27 @@ def test_explicit_timeout_wins_over_the_environment(monkeypatch):
     assert client.timeout.connect == 7.0
 
 
-def test_env_auth_is_still_attached(monkeypatch):
+def test_env_auth_is_still_attached():
     """The factory's original job must survive the change."""
-    _set_env(monkeypatch)
     client = create_async_agentex_client(api_key="test", base_url="http://localhost:5003")
     assert client._client.auth is not None
 
 
-def test_a_bad_value_does_not_prevent_client_creation(monkeypatch):
-    """Timeout configuration must never be the reason a client fails to build."""
-    _set_env(monkeypatch, AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS="not-a-number")
-    client = create_async_agentex_client(api_key="test", base_url="http://localhost:5003")
-    assert client is not None
+def test_a_bad_value_names_the_variable(monkeypatch):
+    """A malformed value is a configuration error, so it must not be swallowed."""
+    monkeypatch.setenv("AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS", "not-a-number")
+    with pytest.raises(ValueError, match="AGENTEX_CLIENT_CONNECT_TIMEOUT_SECONDS"):
+        _timeout_from_env()
+
+
+def test_the_timeout_does_not_depend_on_the_shared_environment_model(monkeypatch):
+    """Regression: these must not become EnvironmentVariables fields.
+
+    That model has required fields, is loaded by worker startup and by
+    EnvAuth.auth_flow on every request, and agentex.lib.adk.utils builds a
+    client at import time. Routing timeouts through it makes all three depend
+    on a fully configured environment.
+    """
+    monkeypatch.delenv("AGENT_NAME", raising=False)
+    monkeypatch.delenv("ACP_URL", raising=False)
+    assert _timeout_from_env().connect == 5.0