feat: consolidate env-var readers on common/env_config (RES-1297)

CHANGELOG.md

@@ -17,6 +17,7 @@ All notable changes to `evaluatorq` are documented here.
 - **A set-but-empty `ORQ_OTEL_*` tuning variable now logs a `WARNING` and falls back to the default, instead of falling back silently.** `_env_int` treated an empty string like an unset variable, so an unresolved workflow variable in a CI `env:` block expands to the empty string and disabled the knob with no signal. Whitespace-only values are treated the same way. Related: `ORQ_OTEL_MAX_BATCH_SIZE` larger than `ORQ_OTEL_MAX_QUEUE_SIZE` is still clamped down to the queue size, but the clamp now announces itself with a `WARNING` rather than happening silently.
 - **`EVALUATORQ_REASONING_EFFORT` has no default — unset means the parameter is not sent, and the model applies its own.** It previously fell back to `"medium"` for the simulator's own calls (user simulator, judge). A global effort is the wrong default in both directions: on a model that does not accept the parameter it costs a rejected request plus a retry per `(model, tool shape)` — memoised per process, so a short run or CI job never amortises it — and on a model that does, it silently overrides the provider's own tuned value. Set the env var, or `LLMCallConfig.reasoning_effort` on the agent's config, when you actually want a specific effort. **Simulation only**; red teaming's `target_reasoning_effort` was already opt-in.
 - **`OrqResponsesTarget.retry_attempts` now defaults to `1` — a single attempt, no retry — down from falling through to `with_retry`'s default of `5`.** `common.target_call.call_target_with_retry` is the single retry owner for target calls on every surface that drives a target (red team static, hybrid, pipeline, orchestrator, and simulation); a target that also retries internally multiplies against that budget instead of adding to it — 5 inner attempts under 3 outer ones is 15 calls to a target that is already refusing. Raise `retry_attempts` only when constructing the target directly and calling `respond()` outside `call_target_with_retry`.
+- **Env-var overrides now share one reader, and a misconfigured `EVALUATORQ_LLM_TIMEOUT_S` / `EVALUATORQ_LLM_MAX_TOKENS` warns and falls back to the default instead of raising.** `common.env_config` (`env_int` / `env_float` / `env_bool`) is now the single place env overrides are parsed and validated: unset falls back to the default silently, and a set-but-empty/whitespace, unparseable, out-of-range, or non-finite value logs a `WARNING` and falls back to the default. It never raises. The private readers in `tracing/setup.py` and `simulation/agents/base.py` route through it. This changes the two simulation knobs above, which previously raised a `ValueError` on a non-numeric value and crashed the process at import (`DEFAULT_MAX_TOKENS` is computed at module scope); they now warn and use the default, matching the non-fatal contract the `ORQ_OTEL_*` tracing knobs already followed, and both are now bounded with `min_value=1` so `0`, a negative, or `nan`/`inf` also fall back with a warning rather than reaching the provider. `ORQ_DISABLE_TRACING` also now recognises `yes` / `on` and is case-insensitive, in addition to the previous `1` / `true`. A shared reader with `env_int` / `env_float` existed briefly on the RES-1286 branch and was removed before merge in favour of pydantic `Field` bounds on the recommendations config (which reject a meaningless value instead of warning and falling back); this reintroduces it deliberately for the process-global tuning knobs, where a warn-and-continue contract is wanted over a hard failure. `EVALUATORQ_SPAN_MAX_TEXT_CHARS`, `EVALUATORQ_CAPTURE_MESSAGE_CONTENT`, `EVALUATORQ_REASONING_EFFORT`, `ORQ_DEBUG` and `COLUMNS` are left as bespoke reads (a capture-all sentinel, a PII gate, a string enum, a truthy-any toggle, and a terminal probe respectively).
 - **`LLMCallConfig.completion_params()` and `.responses_params()` are removed.** Both are replaced by a single `LLMCallConfig.request_params(*, api=None, **params)`, which renders the shape the `api` argument names (`self.api` when omitted). Two builders meant a call site could render a config that says `responses` into chat-completions shape and no one would notice — exactly the accepted-then-ignored failure this class exists to prevent. A call site that is structurally single-endpoint passes `api=` explicitly and gets a warning if that contradicts an explicitly-set `self.api`. **Update any call site using either removed method** — there is no deprecation shim.
 - **Simulation's Responses calls and the executive-summary narrative now go through the canonical executors, and are priced.** `BaseAgent._call_responses` routes through `common.llm_call.execute_response`, and `generate_executive_summary` routes through `execute_chat_completion` — both now get slot limiting, the reasoning drop-and-retry-once, pipeline metadata, trace headers and, previously missing, a `price_usage` call. Simulation Responses calls on non-Orq endpoints were previously left unpriced entirely. `generate_executive_summary` now returns an `ExecutiveSummary(text, usage)` dataclass instead of `str | None` — **update any caller that unpacked or compared the old return value directly.**
 - **Post-processing spend now lands in the run totals instead of only the log.** Red team's `ReportSummary` gains `post_processing_token_usage` (recommendation generation, including trace condensing, plus the executive summary), folded into `token_usage_total` once both steps have run, been skipped, or failed — previously that spend was log-only and invisible to `report.summary`. Simulation's new `SimulationRun.token_usage_total` sums every result's usage plus, for `generate_and_simulate()`, the GENERATE stage's persona/scenario cost and the executive summary's cost; it does **not** include recommendation generation, which stays log-only on both surfaces.

src/evaluatorq/common/env_config.py

@@ -0,0 +1,94 @@
+"""Single contract for reading validated env-var overrides.
+
+One place the package parses and validates environment overrides, so a tuning knob behaves the
+same wherever it is read. The contract:
+
+- unset (variable absent) -> the default, silently.
+- a set-but-invalid value logs a WARNING and falls back to the default. It never raises, so a
+  misconfigured knob is actionable but non-fatal. "Invalid" for a number means: empty/whitespace
+  (in a CI ``env:`` block an unresolved ``${{ vars.X }}`` expands to empty, which should be a
+  signal, not a silent default), unparseable, or outside an optional ``[min_value, max_value]``
+  range. For a bool, empty is treated as unset (-> default).
+
+Prefer ``env_int`` / ``env_float`` / ``env_bool`` over ad hoc ``os.getenv`` + ``int()`` / ``float()``
+in the package.
+"""
+
+from __future__ import annotations
+
+import math
+import os
+
+from loguru import logger
+
+_TRUE = {'1', 'true', 'yes', 'on'}
+_FALSE = {'0', 'false', 'no', 'off'}
+
+
+def _bounded(name: str, value: float, default: float, min_value: float | None, max_value: float | None) -> float:
+    if min_value is not None and value < min_value:
+        logger.warning('{} must be >= {} (got {}); using default {}.', name, min_value, value, default)
+        return default
+    if max_value is not None and value > max_value:
+        logger.warning('{} must be <= {} (got {}); using default {}.', name, max_value, value, default)
+        return default
+    return value
+
+
+def _raw_number(name: str, default: float) -> str | None:
+    """Shared prelude for env_int/env_float: None if unset (silent), else the stripped value;
+    an empty/whitespace value warns and returns None so the caller falls back to the default."""
+    raw = os.environ.get(name)
+    if raw is None:
+        return None
+    raw = raw.strip()
+    if not raw:
+        logger.warning('{} is set but empty; using default {}.', name, default)
+        return None
+    return raw
+
+
+def env_int(name: str, default: int, *, min_value: int | None = None, max_value: int | None = None) -> int:
+    """Read an int override. Unset -> default; empty/invalid/out-of-range -> WARNING + default."""
+    raw = _raw_number(name, default)
+    if raw is None:
+        return default
+    try:
+        value = int(raw)
+    except ValueError:
+        logger.warning('{} is not an integer ({!r}); using default {}.', name, raw, default)
+        return default
+    return int(_bounded(name, value, default, min_value, max_value))
+
+
+def env_float(name: str, default: float, *, min_value: float | None = None, max_value: float | None = None) -> float:
+    """Read a float override. Unset -> default; empty/invalid/out-of-range -> WARNING + default."""
+    raw = _raw_number(name, default)
+    if raw is None:
+        return default
+    try:
+        value = float(raw)
+    except ValueError:
+        logger.warning('{} is not a number ({!r}); using default {}.', name, raw, default)
+        return default
+    if not math.isfinite(value):  # float() accepts nan/inf; a knob is never one of those
+        logger.warning('{} is not a finite number ({!r}); using default {}.', name, raw, default)
+        return default
+    return float(_bounded(name, value, default, min_value, max_value))
+
+
+def env_bool(name: str, *, default: bool) -> bool:
+    """Read a bool override. Unset/empty -> default; unrecognised -> WARNING + default.
+
+    Truthy: 1/true/yes/on. Falsy: 0/false/no/off (case-insensitive).
+    """
+    raw = os.environ.get(name)
+    if raw is None or raw == '':
+        return default
+    value = raw.strip().lower()
+    if value in _TRUE:
+        return True
+    if value in _FALSE:
+        return False
+    logger.warning('{} is not a boolean ({!r}); using default {}.', name, raw, default)
+    return default

src/evaluatorq/simulation/agents/base.py

@@ -12,6 +12,7 @@
 from dataclasses import dataclass, fields
 from typing import TYPE_CHECKING, Any, ClassVar, Literal
 
+from evaluatorq.common.env_config import env_float, env_int
 from evaluatorq.common.llm_call import (
     execute_chat_completion,
     execute_response,
@@ -50,26 +51,6 @@
 logger = logging.getLogger(__name__)
 
 
-def _env_float(name: str, default: float) -> float:
-    raw = os.environ.get(name)
-    if raw is None:
-        return default
-    try:
-        return float(raw)
-    except ValueError:
-        raise ValueError(f'Environment variable {name}={raw!r} must be a number') from None
-
-
-def _env_int(name: str, default: int) -> int:
-    raw = os.environ.get(name)
-    if raw is None:
-        return default
-    try:
-        return int(raw)
-    except ValueError:
-        raise ValueError(f'Environment variable {name}={raw!r} must be an integer') from None
-
-
 # The three functions below resolve at CALL TIME, and are the process-global
 # fallback only: an explicitly set `LLMCallConfig` field always wins.
 
@@ -79,7 +60,7 @@ def _default_timeout_s() -> float:
     tailscale box under parallel load) can exceed the default; raise via
     EVALUATORQ_LLM_TIMEOUT_S, or per-agent via ``LLMCallConfig.timeout_ms``.
     """
-    return _env_float('EVALUATORQ_LLM_TIMEOUT_S', 60.0)
+    return env_float('EVALUATORQ_LLM_TIMEOUT_S', 60.0, min_value=1.0)
 
 
 def _default_max_tokens() -> int:
@@ -90,7 +71,7 @@ def _default_max_tokens() -> int:
     as "no text and no tool calls". Raise via EVALUATORQ_LLM_MAX_TOKENS, or
     per-agent via ``LLMCallConfig.max_tokens``.
     """
-    return _env_int('EVALUATORQ_LLM_MAX_TOKENS', DEFAULT_TARGET_MAX_TOKENS)
+    return env_int('EVALUATORQ_LLM_MAX_TOKENS', DEFAULT_TARGET_MAX_TOKENS, min_value=1)
 
 
 def _default_reasoning_effort() -> str | None:

src/evaluatorq/tracing/setup.py

@@ -41,6 +41,8 @@
 
 from loguru import logger
 
+from evaluatorq.common.env_config import env_bool, env_int
+
 if TYPE_CHECKING:
     from opentelemetry.trace import Tracer
 
@@ -51,37 +53,9 @@
 _initialization_attempted = False
 
 
-def _env_int(name: str, default: int) -> int:
-    """Read a positive int from the environment, falling back to *default*.
-
-    A set-but-invalid value (empty, non-integer or non-positive) logs a WARNING
-    so a misconfigured tuning knob is actionable instead of silently ignored.
-    An empty value counts as set-but-invalid: in a CI ``env:`` block an
-    unresolved ``${{ vars.X }}`` expands to the empty string, which would
-    otherwise fall back to the default with no signal.
-    """
-    raw = os.environ.get(name)
-    if raw is None:
-        return default
-    raw = raw.strip()
-    if not raw:
-        logger.warning('{} is set but empty; using default {}.', name, default)
-        return default
-    try:
-        value = int(raw)
-    except ValueError:
-        logger.warning('{} is not an integer ({!r}); using default {}.', name, raw, default)
-        return default
-    if value <= 0:
-        logger.warning('{} must be positive (got {}); using default {}.', name, value, default)
-        return default
-    return value
-
-
 def _is_tracing_explicitly_disabled() -> bool:
     """Check if tracing is explicitly disabled via ORQ_DISABLE_TRACING."""
-    disable_value = os.environ.get('ORQ_DISABLE_TRACING', '')
-    return disable_value in ('1', 'true')
+    return env_bool('ORQ_DISABLE_TRACING', default=False)
 
 
 def is_tracing_enabled() -> bool:
@@ -219,8 +193,8 @@ async def init_tracing_if_needed() -> bool:  # noqa: RUF029
         # Use BatchSpanProcessor to export spans asynchronously in batches.
         # Env-tunable because a long-lived process never tears the provider down,
         # so one queue absorbs every run and the SDK's 2048 default overflows.
-        max_queue_size = _env_int('ORQ_OTEL_MAX_QUEUE_SIZE', 4096)
-        requested_batch_size = _env_int('ORQ_OTEL_MAX_BATCH_SIZE', 512)
+        max_queue_size = env_int('ORQ_OTEL_MAX_QUEUE_SIZE', 4096, min_value=1)
+        requested_batch_size = env_int('ORQ_OTEL_MAX_BATCH_SIZE', 512, min_value=1)
         batch_size = min(requested_batch_size, max_queue_size)
         if batch_size != requested_batch_size:
             logger.warning(
@@ -233,7 +207,7 @@ async def init_tracing_if_needed() -> bool:  # noqa: RUF029
         span_processor = BatchSpanProcessor(
             exporter,
             max_queue_size=max_queue_size,
-            schedule_delay_millis=_env_int('ORQ_OTEL_SCHEDULE_DELAY_MS', 5000),
+            schedule_delay_millis=env_int('ORQ_OTEL_SCHEDULE_DELAY_MS', 5000, min_value=1),
             max_export_batch_size=batch_size,
         )
 
@@ -286,7 +260,7 @@ async def flush_tracing() -> None:
     if _sdk is None:
         return
     provider = _sdk  # TracerProvider
-    timeout_ms = _env_int('ORQ_OTEL_FLUSH_TIMEOUT_MS', 5000)
+    timeout_ms = env_int('ORQ_OTEL_FLUSH_TIMEOUT_MS', 5000, min_value=1)
     timed_out = 'OTEL span flush timed out after {}ms; some spans may not have been exported.'
     try:
         ok = await asyncio.wait_for(

tests/common/test_env_config.py

@@ -0,0 +1,109 @@
+"""Tests for the shared env-var reader contract (common/env_config).
+
+Covers the return value AND that a WARNING is actually emitted on every invalid case, since a
+silent misconfiguration is the failure this reader exists to prevent.
+"""
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+import pytest
+from loguru import logger
+
+from evaluatorq.common.env_config import env_bool, env_float, env_int
+
+if TYPE_CHECKING:
+    from collections.abc import Iterator
+
+
+@pytest.fixture(autouse=True)
+def _clear(monkeypatch: pytest.MonkeyPatch) -> None:
+    for name in ('X_INT', 'X_FLOAT', 'X_BOOL'):
+        monkeypatch.delenv(name, raising=False)
+
+
+@pytest.fixture
+def warns() -> Iterator[list[str]]:
+    """Capture loguru WARNING messages (loguru does not feed pytest's caplog)."""
+    messages: list[str] = []
+    sink_id = logger.add(lambda m: messages.append(m.record['message']), level='WARNING')
+    yield messages
+    logger.remove(sink_id)
+
+
+# --- env_int ---
+def test_env_int_unset_is_silent(warns: list[str]) -> None:
+    assert env_int('X_INT', 7) == 7
+    assert warns == []  # a truly-absent variable is not a misconfiguration
+
+
+def test_env_int_empty_warns(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_INT', '')
+    assert env_int('X_INT', 7) == 7
+    assert any('set but empty' in m for m in warns)  # unresolved CI ${{ vars.X }} must signal
+
+
+def test_env_int_whitespace_only_warns(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_INT', '   ')
+    assert env_int('X_INT', 7) == 7
+    assert any('set but empty' in m for m in warns)
+
+
+def test_env_int_valid_and_stripped(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_INT', '42')
+    assert env_int('X_INT', 7) == 42
+    monkeypatch.setenv('X_INT', '  42  ')  # surrounding whitespace tolerated
+    assert env_int('X_INT', 7) == 42
+    assert warns == []  # a valid value warns about nothing
+
+
+def test_env_int_invalid_warns_and_defaults(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_INT', 'notanint')
+    assert env_int('X_INT', 7) == 7  # never raises
+    assert any('not an integer' in m for m in warns)
+
+
+def test_env_int_out_of_range_warns_and_defaults(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_INT', '0')
+    assert env_int('X_INT', 7, min_value=1) == 7  # replaces the old "must be positive" check
+    monkeypatch.setenv('X_INT', '999')
+    assert env_int('X_INT', 7, max_value=100) == 7
+    assert sum('must be' in m for m in warns) == 2
+
+
+# --- env_float ---
+def test_env_float_valid_invalid_range(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_FLOAT', '1.5')
+    assert env_float('X_FLOAT', 2.0) == 1.5
+    monkeypatch.setenv('X_FLOAT', 'nope')
+    assert env_float('X_FLOAT', 2.0) == 2.0
+    monkeypatch.setenv('X_FLOAT', '-1')
+    assert env_float('X_FLOAT', 2.0, min_value=0.0) == 2.0
+    assert any('not a number' in m for m in warns)
+    assert any('must be >=' in m for m in warns)
+
+
+@pytest.mark.parametrize('raw', ['nan', 'inf', '-inf', 'Infinity'])
+def test_env_float_rejects_non_finite(monkeypatch: pytest.MonkeyPatch, warns: list[str], raw: str) -> None:
+    monkeypatch.setenv('X_FLOAT', raw)
+    assert env_float('X_FLOAT', 2.0) == 2.0  # float() would accept these; the reader must not
+    assert any('finite' in m for m in warns)
+
+
+# --- env_bool ---
+@pytest.mark.parametrize(('raw', 'expected'), [('1', True), ('true', True), ('YES', True), ('on', True), ('0', False), ('false', False), ('no', False), ('OFF', False)])
+def test_env_bool_recognised(monkeypatch: pytest.MonkeyPatch, raw: str, expected: bool) -> None:
+    monkeypatch.setenv('X_BOOL', raw)
+    assert env_bool('X_BOOL', default=not expected) is expected
+
+
+def test_env_bool_unset_is_silent(warns: list[str]) -> None:
+    assert env_bool('X_BOOL', default=True) is True
+    assert warns == []
+
+
+def test_env_bool_unrecognised_warns_and_defaults(monkeypatch: pytest.MonkeyPatch, warns: list[str]) -> None:
+    monkeypatch.setenv('X_BOOL', 'maybe')
+    assert env_bool('X_BOOL', default=True) is True  # unrecognised -> warn + default, never raises
+    assert any('not a boolean' in m for m in warns)