fix(audit): remediate whole-project audit findings

.env.example

@@ -18,20 +18,25 @@ LLM_MODEL=gemini-3.1-flash-lite
 GEMINI_API_KEY=your_gemini_api_key_here
 OPENAI_API_KEY=your_openai_api_key_here
 
+# Retry policy for transient LLM failures (429 / 5xx / network). Non-transient
+# errors such as 401 or 400 are never retried.
+LLM_MAX_RETRIES=3
+LLM_RETRY_BASE_DELAY_S=0.5
+LLM_RETRY_MAX_DELAY_S=10
+
 # External Tools & Sandboxing
 SEARCH_API_KEY=your_search_api_key_here
 FILE_TOOL_BASE_DIR=/tmp/agentkit_sandbox
 
 # Security & API Authentication
-# Comma-separated list of authorized client API keys.
-# docker-compose.yml ships no default: leave this unset and the api service
-# exits on settings validation instead of starting with a published key.
-API_KEYS=ak_test_key_12345,ak_admin_key_67890
+# Comma-separated list of authorized client API keys. Required: AgentKit refuses
+# to start when this is empty or unset, so no deployment can come up with a key
+# published in this repository. Replace the values below with your own.
+API_KEYS=replace_me_key_1,replace_me_key_2
 RATE_LIMIT_PER_MIN=30
 
 # Agent Execution Limits & Guardrails
 MAX_STEPS=10
-MAX_RETRIES=3
 RUN_TIMEOUT_S=60
 TOOL_TIMEOUT_S=15
 SESSION_TTL_S=3600

README.md

@@ -110,16 +110,22 @@ OPENAI_API_KEY=your_openai_api_key_here
 DATABASE_URL=postgresql+asyncpg://agentkit:agentkit_password@localhost:5432/agentkit
 REDIS_URL=redis://localhost:6379/0
 
-# Security (comma-separated authorized keys)
-API_KEYS=ak_test_key_12345,ak_admin_key_67890
+# Security (API_KEYS is required; the app refuses to start without it)
+API_KEYS=dev_key_1,dev_key_2
+
+# Optional: retry policy for transient LLM failures (429/5xx/network)
+LLM_MAX_RETRIES=3
+LLM_RETRY_BASE_DELAY_S=0.5
+LLM_RETRY_MAX_DELAY_S=10
 ```
 
 ### 4. Running with Docker Compose (Recommended)
 Launch the API, PostgreSQL, and Redis together with automatic migrations.
 
-`docker-compose.yml` ships no default `API_KEYS`. Leave it unset and the api service
-fails immediately on settings validation rather than starting with a published key;
-`down`, `ps` and `logs` still work either way.
+`API_KEYS` is required in every environment, not just under compose. `Settings` rejects an
+empty value, so a deployment that forgets to configure its keys fails at startup rather than
+serving unauthenticated traffic with a key published in the repository. `docker-compose.yml`
+ships no default either; `down`, `ps` and `logs` still work without it.
 
 ```bash
 export API_KEYS=ak_local_dev_key   # or place API_KEYS in your .env

agentkit/api/deps.py

@@ -138,16 +138,26 @@ def get_llm_client(
     request: Request,
     settings: Settings = Depends(get_current_settings),
 ) -> LLMClient:
-    """Resolve configured LLM client.
+    """Resolve the configured LLM client.
 
-    Instantiates the configured provider from settings. Explicit test overrides
-    can be supplied via request.app.state.llm_client.
+    Instantiates the configured provider from settings, which also applies the
+    configured transient-error retry policy. Explicit test overrides can be supplied
+    via request.app.state.llm_client. A provider construction failure is surfaced as a
+    503: falling back to a scripted client would answer a misconfigured deployment with
+    fake text and HTTP 200 instead of surfacing the misconfiguration.
     """
-    if hasattr(request.app.state, "llm_client") and request.app.state.llm_client is not None:
-        client: LLMClient = request.app.state.llm_client
-        return client
-
-    return create_llm_client_from_settings(settings)
+    override: LLMClient | None = getattr(request.app.state, "llm_client", None)
+    if override is not None:
+        return override
+
+    try:
+        return create_llm_client_from_settings(settings)
+    except Exception as exc:
+        logger.error("Failed to initialize LLM provider %s: %s", settings.LLM_PROVIDER, exc)
+        raise ServiceUnavailableError(
+            "llm",
+            f"LLM provider '{settings.LLM_PROVIDER}' could not be initialized.",
+        ) from exc
 
 
 def get_agent(
@@ -158,10 +168,15 @@ def get_agent(
     memory: MemoryStore = Depends(get_memory_store),
     trace: TraceSink = Depends(get_trace_sink),
 ) -> Agent:
-    """Assemble configured Agent instance with all runtime dependencies."""
-    if hasattr(request.app.state, "agent") and request.app.state.agent is not None:
-        agent: Agent = request.app.state.agent
-        return agent
+    """Assemble a configured Agent from the currently resolved dependencies.
+
+    The agent is rebuilt per request rather than memoized on ``app.state``: caching the first
+    assembled instance would pin every later request to that request's LLM client, registry,
+    memory store and trace sink, silently ignoring any later override or settings change.
+    """
+    override: Agent | None = getattr(request.app.state, "agent_override", None)
+    if override is not None:
+        return override
 
     config = AgentConfig(
         max_steps=settings.MAX_STEPS,

agentkit/api/ratelimit.py

@@ -10,7 +10,7 @@
 
 from fastapi import Depends
 from redis.asyncio import Redis
-from redis.exceptions import RedisError, ResponseError
+from redis.exceptions import RedisError, ResponseError, WatchError
 
 from agentkit.api.auth import get_current_settings, verify_api_key
 from agentkit.api.deps import get_redis_client
@@ -57,6 +57,10 @@
 return {1, 0}
 """
 
+#: Retries for the WATCH/MULTI fallback when concurrent writers keep invalidating the watch.
+#: Only reached on Redis deployments without scripting support.
+_WATCH_MAX_RETRIES = 5
+
 
 class RateLimiter:
     """Sliding window rate limiter enforcing request volume limits per client key.
@@ -203,23 +207,48 @@ async def _check_window_unatomic(
         member: str,
         key_ttl: int,
     ) -> tuple[bool, int]:
-        """Best-effort sliding-window check for servers without scripting support."""
-        pipe = redis_client.pipeline(transaction=True)
-        pipe.zremrangebyscore(redis_key, "-inf", now - window)
-        pipe.zcard(redis_key)
-        results = await pipe.execute()
-        if int(results[1]) >= limit:
-            oldest = await redis_client.zrange(redis_key, 0, 0, withscores=True)
-            retry_after = (
-                max(1, math.ceil(float(oldest[0][1]) + window - now)) if oldest else int(window)
-            )
-            return False, retry_after
+        """Best-effort sliding-window check for servers without scripting support.
+
+        An earlier version trimmed and counted in one transaction, then inserted in a second.
+        That second round trip leaves a gap in which concurrent requests all read the same
+        pre-insert cardinality and are admitted together, so a parallel burst could exceed the
+        quota. WATCH/MULTI/EXEC closes the gap: the whole read-decide-write cycle aborts and
+        retries whenever another client modifies the key in between.
+        """
+        for _ in range(_WATCH_MAX_RETRIES):
+            async with redis_client.pipeline(transaction=True) as pipe:
+                try:
+                    await pipe.watch(redis_key)
+                    await pipe.zremrangebyscore(redis_key, "-inf", now - window)
+                    count = await pipe.zcard(redis_key)
 
-        pipe = redis_client.pipeline(transaction=True)
-        pipe.zadd(redis_key, {member: now})
-        pipe.expire(redis_key, key_ttl)
-        await pipe.execute()
-        return True, 0
+                    if int(count) >= limit:
+                        oldest = await pipe.zrange(redis_key, 0, 0, withscores=True)
+                        await pipe.reset()
+                        retry_after = (
+                            max(1, math.ceil(float(oldest[0][1]) + window - now))
+                            if oldest
+                            else int(window)
+                        )
+                        return False, retry_after
+
+                    pipe.multi()
+                    pipe.zadd(redis_key, {member: now})
+                    pipe.expire(redis_key, key_ttl)
+                    await pipe.execute()
+                    return True, 0
+                except WatchError:
+                    # Another client changed the window between WATCH and EXEC; re-read and retry.
+                    continue
+
+        # Contention never settled. Denying is the safe direction for a limiter: a false
+        # reject costs one request, a false admit lets a client exceed its quota.
+        logger.warning(
+            "Rate limit check for key %s hit %d watch retries under contention; denying request.",
+            redis_key[:8] + "...",
+            _WATCH_MAX_RETRIES,
+        )
+        return False, max(1, math.ceil(window))
 
     async def __call__(
         self,

agentkit/api/routes.py

@@ -31,6 +31,11 @@
 
 logger = logging.getLogger(__name__)
 
+#: Fixed client-facing strings for failed dependency checks. Raw driver errors can contain
+#: connection URLs, credentials or internal hostnames, and /health requires no API key.
+DATABASE_UNHEALTHY = "Database connectivity check failed."
+REDIS_UNHEALTHY = "Redis connectivity check failed."
+
 runs_router = APIRouter(
     prefix="/runs",
     tags=["runs"],
@@ -47,7 +52,10 @@ async def create_run(
     memory_store: PostgresMemoryStore | MemoryStore = Depends(get_memory_store),
 ) -> RunResponse:
     """Execute an agent goal and return execution summary."""
-    logger.info("Executing run for goal: %s (session_id: %s)", payload.goal, payload.session_id)
+    # The goal is caller-supplied prompt data, so only its length is logged at INFO.
+    logger.info(
+        "Executing run (goal_chars=%d, session_id: %s)", len(payload.goal), payload.session_id
+    )
     result = await agent.run(goal=payload.goal, session_id=payload.session_id)
 
     if hasattr(memory_store, "get_run"):
@@ -153,31 +161,33 @@ async def health_check(request: Request) -> JSONResponse:
     redis_status = "healthy"
     details: dict[str, str] = {}
 
-    # Check Database Engine
+    # /health is unauthenticated, so connection error strings are logged rather than echoed:
+    # driver messages routinely embed host, port and database name.
     engine = getattr(request.app.state, "db_engine", None)
     if engine is not None:
         try:
             async with engine.connect() as conn:
                 await conn.execute(text("SELECT 1"))
         except Exception as exc:
             db_status = "unhealthy"
-            details["database"] = str(exc)
+            details["database"] = DATABASE_UNHEALTHY
+            logger.warning("Database health check failed: %s", exc)
             status_code = 503
     else:
         db_status = "disabled"
 
-    # Check Redis Client
     redis_client = getattr(request.app.state, "redis_client", None)
     if redis_client is not None:
         try:
             ping_res = await redis_client.ping()
             if not ping_res:
                 redis_status = "unhealthy"
-                details["redis"] = "Ping returned False"
+                details["redis"] = REDIS_UNHEALTHY
                 status_code = 503
         except Exception as exc:
             redis_status = "unhealthy"
-            details["redis"] = str(exc)
+            details["redis"] = REDIS_UNHEALTHY
+            logger.warning("Redis health check failed: %s", exc)
             status_code = 503
     else:
         redis_status = "disabled"

agentkit/config.py

@@ -49,15 +49,31 @@ class Settings(BaseSettings):
         default=None,
         description="External search API key (Tavily/SerpAPI).",
     )
+    LLM_MAX_RETRIES: int = Field(
+        default=3,
+        ge=0,
+        le=10,
+        description="Retry attempts for transient LLM provider failures (429/5xx/network).",
+    )
+    LLM_RETRY_BASE_DELAY_S: float = Field(
+        default=0.5,
+        gt=0,
+        description="Base delay in seconds for exponential LLM retry backoff.",
+    )
+    LLM_RETRY_MAX_DELAY_S: float = Field(
+        default=10.0,
+        gt=0,
+        description="Maximum delay in seconds between LLM retry attempts.",
+    )
     FILE_TOOL_BASE_DIR: str = Field(
         default="/tmp/agentkit_sandbox",
         description="Base directory for sandboxed file reading operations.",
     )
 
     # Security & API Authentication
     API_KEYS: str = Field(
-        default="ak_test_key_12345",
-        description="Comma-separated list of valid client API keys.",
+        default="",
+        description="Comma-separated list of valid client API keys. Must be set explicitly.",
     )
     RATE_LIMIT_PER_MIN: int = Field(
         default=30,
@@ -71,11 +87,6 @@ class Settings(BaseSettings):
         gt=0,
         description="Maximum execution steps allowed per agent run.",
     )
-    MAX_RETRIES: int = Field(
-        default=3,
-        ge=0,
-        description="Maximum retries on transient LLM errors.",
-    )
     RUN_TIMEOUT_S: int = Field(
         default=60,
         gt=0,
@@ -100,10 +111,17 @@ class Settings(BaseSettings):
     @field_validator("API_KEYS")
     @classmethod
     def validate_api_keys(cls, v: str) -> str:
-        """Ensure API keys are not empty."""
+        """Reject blank or placeholder keys so a deployment cannot start unauthenticated.
+
+        The default is intentionally empty: starting with a key published in the repository
+        would expose any deployment that forgets to configure its own.
+        """
         cleaned = v.strip()
         if not cleaned:
-            raise ValueError("API_KEYS must not be empty")
+            raise ValueError(
+                "API_KEYS must be set to a comma-separated list of client API keys. "
+                "AgentKit refuses to start without explicit authentication credentials."
+            )
         return cleaned
 
     @property

agentkit/core/agent.py

@@ -142,7 +142,10 @@ async def run(
         run_id = str(uuid.uuid4())
 
         with log_context(run_id=run_id):
-            logger.info("Starting agent run for goal: %s", goal)
+            # The goal is caller-supplied prompt data, so it stays out of INFO per the
+            # logging policy; only its length is recorded there.
+            logger.info("Starting agent run (goal_chars=%d, session_id=%s)", len(goal), session_id)
+            logger.debug("Run goal content: %s", goal)
             if self.memory is not None and hasattr(self.memory, "create_run"):
                 with contextlib.suppress(Exception):
                     await self.memory.create_run(

agentkit/llm/factory.py

@@ -76,10 +76,12 @@ def create_llm_client_from_settings(settings: Settings) -> LLMClient:
         model=settings.LLM_MODEL,
     )
 
-    if provider in ("gemini", "openai") and settings.MAX_RETRIES > 0:
+    if provider in ("gemini", "openai") and settings.LLM_MAX_RETRIES > 0:
         return RetryingLLMClient(
             client=client,
-            max_retries=settings.MAX_RETRIES,
+            max_retries=settings.LLM_MAX_RETRIES,
+            base_delay=settings.LLM_RETRY_BASE_DELAY_S,
+            max_delay=settings.LLM_RETRY_MAX_DELAY_S,
         )
 
     return client