feat: consolidate env-var readers on common/env_config (RES-1297) by KarinaKKarinaK · Pull Request #188 · orq-ai/evaluatorq

KarinaKKarinaK · 2026-09-03T08:18:41Z

Retire the private env-var readers and route the numeric/bool overrides through one shared, validated reader.

Framing (corrected after review)

An earlier version of this description called the ticket wrong for assuming common/env_config.py existed. That was my error. A shared reader with env_int/env_float did exist briefly on the RES-1286 branch (ff0e4049) and was removed before merge (bf3e88ee) in favour of pydantic Field bounds on the recommendations config, which reject a meaningless value outright rather than warning and falling back. So this PR is a deliberate reintroduction of that reader for the process-global tuning knobs, where a warn-and-continue contract is wanted over a hard failure, not a correction to a mistaken ticket. Both spellings are the right call in their place: Field bounds for a config a caller constructs, a warn-and-fallback reader for an env knob you do not want to crash the process.

What this does

Adds common/env_config.py with env_int / env_float (optional min_value/max_value) and env_bool, on one contract: unset -> default silently; set-but-empty/whitespace, unparseable, out-of-range, or non-finite -> WARNING + default; never raises. Truthy 1/true/yes/on, falsy 0/false/no/off, case-insensitive.
Routes tracing/setup.py (the four ORQ_OTEL_* ints via env_int(min_value=1), ORQ_DISABLE_TRACING via env_bool) and simulation/agents/base.py (EVALUATORQ_LLM_TIMEOUT_S, EVALUATORQ_LLM_MAX_TOKENS, both min_value=1) through it, and deletes the private readers.

Scope

Numeric/bool parsers only. String reads (ORQ_API_KEY, ORQ_BASE_URL) are left as-is. Left deliberately with a reason each: EVALUATORQ_SPAN_MAX_TEXT_CHARS (Optional int with a <=0 = capture-all sentinel), EVALUATORQ_CAPTURE_MESSAGE_CONTENT (PII gate, bespoke opt-out), EVALUATORQ_REASONING_EFFORT (string enum), ORQ_DEBUG (truthy-any toggle), COLUMNS (terminal probe).

Behavior change (documented in CHANGELOG)

A misconfigured EVALUATORQ_LLM_TIMEOUT_S / EVALUATORQ_LLM_MAX_TOKENS now warns and falls back instead of raising and crashing at import (DEFAULT_MAX_TOKENS is computed at module scope). Both are bounded min_value=1, so 0, a negative, or nan/inf fall back with a warning rather than reaching the provider. ORQ_DISABLE_TRACING also accepts yes/on and is case-insensitive. The tracing knobs' empty-string warning and positive-only checks are preserved.

Tests

tests/common/test_env_config.py covers the contract and asserts a WARNING is emitted on every invalid case (empty, whitespace, unparseable, out-of-range, non-finite, unrecognised bool), with unset kept silent. Removed-private-reader tests are rewritten against the new contract. basedpyright clean (whole repo), ruff check src / ruff format --check src clean, pytest -m 'not integration' green.

Closes RES-1297

One contract for validated env-var overrides: unset/empty -> default, invalid or out-of-range -> WARNING + default (never raises). env_int / env_float (optional min/max bounds) and env_bool (1/true/yes/on vs 0/false/no/off). This is the module the private readers in tracing/setup and simulation/agents/base will route through. 14 tests, ruff clean.

… (RES-1297) tracing/setup and simulation/agents/base kept their own env-int/float readers with different contracts (tracing warned and fell back and enforced positive; simulation raised on invalid with no range check). Both now use common.env_config: env_int with min_value=1 for the ORQ_OTEL_* knobs, env_int/env_float for the simulation timeout and max-tokens, and env_bool for ORQ_DISABLE_TRACING. Behavior change (documented in CHANGELOG): a bad EVALUATORQ_LLM_TIMEOUT_S / EVALUATORQ_LLM_MAX_TOKENS now warns and falls back instead of raising; ORQ_DISABLE_TRACING also accepts yes/on and is case-insensitive. Empty-string warning and positive-only checks for the tracing knobs are preserved. Tests for the removed private readers are rewritten against the new contract / moved to test_env_config.

github-actions · 2026-09-03T08:22:52Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/evaluatorq/common
env_config.py
src/evaluatorq/simulation/agents
base.py
src/evaluatorq/tracing
setup.py
Project Total

_{This report was generated by python-coverage-comment-action}

currentlycodinng

The refactor itself is right and the mechanics are clean: all four ORQ_OTEL_* ints route through env_int(min_value=1), which is equivalent to the old value <= 0 reject for ints, ORQ_DISABLE_TRACING goes through env_bool, and grep -rn '_env_int|_env_float' src/ confirms both private readers are actually gone rather than orphaned. ruff, basedpyright and 5239 tests are green for me too, and I could not reproduce the test_framework_targets_exposed.py flake in two runs (once with -p no:randomly), so that caveat may be stale.

Three things before this goes in.

The empty-string warning is not preserved, and CHANGELOG.md contradicts itself about it in the same unreleased section. Line 17 (a prior, unrelated entry) documents that a set-but-empty ORQ_OTEL_* variable now warns instead of falling back silently, specifically for the CI ${{ vars.X }}-expands-to-empty case. Line 20, this PR's own entry, claims the new shared reader preserves that: 'an empty/whitespace, unparseable, or out-of-range value logs a WARNING'. It does not, for empty. env_config.py:38 (and :51, :67) short-circuit on raw == '' before any logging:

empty string ''  -> 7  warn=''
whitespace '   ' -> 7  warn="X is not an integer ('   ')..."

On main the old tracing reader gave 'ORQ_OTEL_MAX_QUEUE_SIZE is set but empty; using default 4096.'; on this PR it's silent. That's a straight revert of the fix line 17 describes. Split the empty branch out and warn on it, or fix the CHANGELOG line to say what the code does and drop the now-false claim.

No test asserts a warning is emitted anywhere in this PR. test_env_int_invalid_warns_and_defaults only checks the return value, same pattern across the file. The test you deleted, test_env_int_warns_on_set_but_invalid_value, did assert warning.assert_called_once() over ['', ' ', 'invalid', '0', '-1'], and the comment replacing it at test_tracing_lifecycle.py:134 claims that contract is 'covered in tests/common/test_env_config.py'. It is not, and that lost assertion is exactly why the empty-string regression above got through untested.

Bound the two LLM knobs. Making them non-fatal is fine by me, and the old failure was worse than the description says: DEFAULT_MAX_TOKENS is computed at module scope (base.py:103), so a bad value raised at import time, not call time, in the old code. But you built min_value for exactly this kind of guard and used it only on the tracing knobs. Neither LLM call site passes one, so EVALUATORQ_LLM_MAX_TOKENS=0 and EVALUATORQ_LLM_TIMEOUT_S=-5 now sail through to the provider silently, and env_float parses 'nan' and 'inf' without complaint too. Removing the hard stop is only safe if the range check replaces it. min_value=1 on both.

Separately, on the framing. git log --all -S'def env_int' finds ff0e404 on the #141 branch, which added common/env_config.py with env_int/env_float, and bf3e88e, later in that same PR, which deleted it. The ticket was not wrong, env_config existed there and was consolidated away before merge. That commit's message says why: pydantic Field bounds reject values that cannot mean anything (a cap of 0 pays for the call, drops every result) instead of falling back with a warning, and the rationale still lives in common/recommendations.py:31. So this reintroduces a module a previous review removed and flips its contract back. A shared reader for these knobs still seems like the right call to me, but say that in the description as a reversal you're making on purpose, not as a correction to a mistaken ticket. Someone will read the two commits side by side eventually.

Scope calls all check out. EVALUATORQ_SPAN_MAX_TEXT_CHARS really does use value if value > 0 else None as a capture-all sentinel (common/tracing.py:63), which env_int(min_value=...) would have destroyed, correctly left alone, same for the other three. EVALUATORQ_REASONING_EFFORT (base.py:96) is a fourth bare read in a file you're editing, worth a line in the scope list.

currentlycodinng · 2026-09-03T08:36:12Z

+def env_int(name: str, default: int, *, min_value: int | None = None, max_value: int | None = None) -> int:
+    """Read an int override. Unset/empty -> default; invalid or out-of-range -> WARNING + default."""
+    raw = os.environ.get(name)
+    if raw is None or raw == '':


Empty string returns the default with no warning here, and the same short-circuit is at :51 and :67. CHANGELOG.md:20 says empty warns. Verified it does not:

empty string '' -> 7 warn='' whitespace ' ' -> 7 warn="X is not an integer (' ')..."

On main the old tracing reader gave ORQ_OTEL_MAX_QUEUE_SIZE is set but empty; using default 4096. This reverts CHANGELOG.md:17, which documents that fix and the unresolved-CI-variable case it exists for. Split the empty branch out and warn on it before falling back.

currentlycodinng · 2026-09-03T08:36:12Z

    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)


No min_value here or on the timeout at :63, so EVALUATORQ_LLM_MAX_TOKENS=0 is accepted and forwarded to the provider. Previously the raise at least stopped a nonsense value; now nothing does. You built min_value for exactly this and used it only on the tracing knobs. min_value=1 on both, please.

currentlycodinng · 2026-09-03T08:36:12Z

    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)


Same here, plus env_float parses 'nan' and 'inf' with no warning (verified, both returned as-is), and -5 is accepted as a timeout. min_value=1 (or some floor) would cover all three.

currentlycodinng · 2026-09-03T08:36:12Z

+    assert env_int('X_INT', 7) == 42
+
+
+def test_env_int_invalid_warns_and_defaults(monkeypatch: pytest.MonkeyPatch) -> None:


Named ..._warns_and_defaults but nothing asserts a warning. Same across the file, no test in this PR checks that a warning is emitted at all. The deleted test_env_int_warns_on_set_but_invalid_value did (warning.assert_called_once() over ['', ' ', 'invalid', '0', '-1']), and that lost assertion is why the empty-string regression got through.

currentlycodinng · 2026-09-03T08:36:12Z

+# --- env_int ---
+def test_env_int_unset_and_empty_use_default(monkeypatch: pytest.MonkeyPatch) -> None:
+    assert env_int('X_INT', 7) == 7
+    monkeypatch.setenv('X_INT', '')


This pins the silent-empty behaviour as intended, which is the opposite of what CHANGELOG.md:20 claims for this contract. Once empty warns, split this into an unset case and an empty case that asserts the warning.

currentlycodinng · 2026-09-03T08:36:12Z

-
-    assert tracing_setup._env_int('X', 4096) == 4096
-    warning.assert_called_once()
+# The ORQ_OTEL_* int knobs read through common.env_config.env_int (min_value=1); the reader's


"the reader's contract (unset/empty/invalid/non-positive -> default + warning) is covered in tests/common/test_env_config.py" - the values are covered, the warning is not asserted anywhere in that file, and the empty case does not warn at all. Either the comment or the new tests need to change.

currentlycodinng · 2026-09-03T08:36:12Z

 - **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, and an empty/whitespace, unparseable, or out-of-range 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 where the fallback resolved; they now warn and use the default, matching the non-fatal contract the `ORQ_OTEL_*` tracing knobs already followed. `ORQ_DISABLE_TRACING` also now recognises `yes` / `on` and is case-insensitive, in addition to the previous `1` / `true`.


"an empty/whitespace, unparseable, or out-of-range value logs a WARNING" is not accurate for empty. It also contradicts the entry two lines above at :17 in the same unreleased section, which says a set-but-empty ORQ_OTEL_* variable warns instead of falling back silently. Whichever way the code lands, these two entries need to agree.

… (RES-1297) Addresses review on #188: - env_config now warns on a set-but-empty/whitespace value instead of silently defaulting (restores the tracing reader's contract; the empty case is the unresolved-CI-variable signal), and env_float rejects nan/inf. - EVALUATORQ_LLM_TIMEOUT_S / EVALUATORQ_LLM_MAX_TOKENS now pass min_value=1, so 0, negatives and non-finite fall back with a warning rather than reaching the provider (the old raise had at least stopped them). - Tests now assert a WARNING is emitted on every invalid case (the lost assertion that let the empty regression through), split unset (silent) from empty (warns), and cover nan/inf. - CHANGELOG reframed: this reintroduces a shared reader RES-1286 had and removed before merge, deliberately, for the process-global knobs; lists the vars left as bespoke reads. No em dashes.

KarinaKKarinaK · 2026-09-03T08:59:29Z

Thanks, all five landed in ea22cda.

Empty-string warning: you were right, the committed reader short-circuited on empty before logging (my warn-on-empty fix was sitting unstaged, so what I reported as verified was my working tree, not the PR). env_int/env_float now warn "set but empty" on empty/whitespace and fall back, restoring the tracing contract and the unresolved-CI-variable signal. CHANGELOG line now matches, and no longer contradicts the entry above it.
Bounds: EVALUATORQ_LLM_TIMEOUT_S and EVALUATORQ_LLM_MAX_TOKENS now pass min_value=1, and env_float rejects nan/inf (float() accepts them and the min/max checks can't catch a nan since every comparison is False). So 0, negatives and non-finite fall back with a warning instead of reaching the provider. Good catch on the import-time raise too, noted in the CHANGELOG.
Warning assertions: added. Every invalid case now asserts a WARNING fired (empty, whitespace, unparseable, out-of-range, non-finite, unrecognised bool), unset is asserted silent, and the empty and nan/inf cases are covered. That was the lost assertion that let the regression through.
Framing: corrected. You're right that env_config existed on the RES-1286 branch (ff0e404) and was removed before merge (bf3e88e) for the Field-bounds reason. The PR body and CHANGELOG now describe this as a deliberate reintroduction of a warn-and-fallback reader for the process-global knobs, not a ticket correction. I also fixed the same wrong framing on the Linear ticket.
EVALUATORQ_REASONING_EFFORT added to the left-alone scope list (base.py:96, a string enum).

basedpyright/ruff/tests green. Ready for another look.

Baukebrenninkmeijer

🤖 Automated review run by Bauke's agent — ping me if something looks off.

3.5/5 — one shared reader, a real contract, and a test file that asserts the warning on every invalid case; the ticket's inventory step misses one env read.

I read the whole diff and checked the contract against each input class: unset, empty, whitespace, 1e3, a float given to env_int, nan, inf, a negative, a value at min_value, and an unrecognised or mixed-case bool. Every invalid path logs a WARNING and returns the default, and none of them raises. All 14 CI checks pass: Lint, Typecheck + test on Python 3.10 to 3.13, and SDK compatibility on three versions.

P1

One numeric env read stays ad hoc and is not on the list of deliberate exceptions (src/evaluatorq/common/model_catalogue.py:122) — _CATALOGUE_TIMEOUT_S = float(os.environ.get('EVALUATORQ_CATALOGUE_TIMEOUT_S', '30')) runs at module scope. A non-numeric value raises ValueError at import, which is the failure this PR removes from simulation/agents/base.py. The Scope section of the PR body names five bespoke reads and does not name this one. Route it through env_float, or add it to the list with a reason.

P2

env_bool does not strip before it tests for empty (src/evaluatorq/common/env_config.py:86) — ORQ_DISABLE_TRACING=' ' warns is not a boolean. env_int and env_float call _raw_number, which strips first and warns is set but empty. Strip in env_bool too.
Two tests promise a warning that they do not assert (tests/simulation/test_agents_base_env.py:27) — test_max_tokens_garbage_warns_and_defaults and test_timeout_garbage_warns_and_defaults assert the default only. Add the warning assertion, or drop warns from the name.
The module docstring lists two truthy words where the code now accepts four (src/evaluatorq/tracing/setup.py:56) — ORQ_DISABLE_TRACING also accepts yes and on, and the match is case-insensitive.
No test covers the new min_value=1 bound on the two simulation knobs (tests/simulation/test_agents_base_env.py:41) — EVALUATORQ_LLM_MAX_TOKENS=0 and EVALUATORQ_LLM_TIMEOUT_S=-5 are a documented behaviour change with no case in this file.

Baukebrenninkmeijer · 2026-09-03T10:28:07Z

+    raw = os.environ.get(name)
+    if raw is None or raw == '':
+        return default
+    value = raw.strip().lower()


P2 — this tests for empty before it strips, so a whitespace-only value warns is not a boolean. _raw_number strips first and warns is set but empty for the same input. Strip here too.

Baukebrenninkmeijer · 2026-09-03T10:28:07Z

-    monkeypatch.setenv("EVALUATORQ_TEST_INT", "abc")
-    with pytest.raises(ValueError, match="EVALUATORQ_TEST_INT"):
-        _env_int("EVALUATORQ_TEST_INT", 8192)
+def test_max_tokens_garbage_warns_and_defaults(monkeypatch: pytest.MonkeyPatch) -> None:


P2 — the name promises a warning that the test does not assert. Add the warning assertion, or drop warns from the name. The same applies to test_timeout_garbage_warns_and_defaults.

KarinaKKarinaK added 2 commits September 3, 2026 16:04

KarinaKKarinaK requested review from Baukebrenninkmeijer, arianpasquali and currentlycodinng as code owners September 3, 2026 08:18

KarinaKKarinaK self-assigned this Sep 3, 2026

github-actions Bot added the feature New feature label Sep 3, 2026

currentlycodinng requested changes Sep 3, 2026

View reviewed changes

Baukebrenninkmeijer reviewed Sep 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

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

feat: consolidate env-var readers on common/env_config (RES-1297)#188
KarinaKKarinaK wants to merge 3 commits into
mainfrom
karinakalicka/res-1297-consolidate-env-var-readers-on-commonenv_config-in

KarinaKKarinaK commented Sep 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 3, 2026 •

edited

Loading

Uh oh!

currentlycodinng left a comment

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

currentlycodinng Sep 3, 2026

Uh oh!

KarinaKKarinaK commented Sep 3, 2026

Uh oh!

Baukebrenninkmeijer left a comment

Uh oh!

Baukebrenninkmeijer Sep 3, 2026

Uh oh!

Baukebrenninkmeijer Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		assert env_int('X_INT', 7) == 42


		def test_env_int_invalid_warns_and_defaults(monkeypatch: pytest.MonkeyPatch) -> None:

Uh oh!

Conversation

KarinaKKarinaK commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Framing (corrected after review)

What this does

Scope

Behavior change (documented in CHANGELOG)

Tests

Uh oh!

github-actions Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

currentlycodinng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarinaKKarinaK commented Sep 3, 2026

Uh oh!

Baukebrenninkmeijer left a comment

Choose a reason for hiding this comment

P1

P2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KarinaKKarinaK commented Sep 3, 2026 •

edited

Loading

github-actions Bot commented Sep 3, 2026 •

edited

Loading