Repository navigation
fix(sdk/python): parse AGENTFIELD_ASYNC boolean flags with the repo's env vocabulary - #1082
Conversation
… env vocabulary AsyncConfig.from_environment() read all five boolean feature flags with `lambda x: x.lower() == "true"`. Four of those fields default to True, so the only values that could be expressed were "true" and "false": setting AGENTFIELD_ASYNC_ENABLE_RESULT_CACHING=1 -- or =yes, or =on -- evaluated to False and silently disabled result caching, the opposite of what the value says. The same inversion applied to enable_async_execution, enable_batch_polling and fallback_to_sync, and it reaches users by default because both Agent (agent.py:846) and AgentFieldClient (client.py:182) build their async_config from from_environment() when none is passed. The lambda also never raises, so get_env_var's `except (ValueError, TypeError): return default_value` fallback is unreachable for booleans and an unparseable value overrides the field with False instead of leaving the default. PR Agent-Field#714, which wired this into the client default, documents the intended contract: from_environment() "only overrides fields when the corresponding env var is set (falling back to the default on unparseable values)". Use the vocabulary the package already uses, split by the field's default: default-on flags opt out with ("0", "false", "no", "off") like log_writer._queue_enabled, logger._stdout_mirror_enabled, node_logs.logs_enabled and openrouter_attribution.attribution_enabled; the default-off enable_event_stream opts in with ("1", "true", "yes", "on") like litellm_observability._TRUE_VALUES. A value outside the vocabulary now leaves the field at its default, which is what makes the documented fallback hold without the converter having to raise. With no AGENTFIELD_ASYNC_* variable set the converters are never reached -- get_env_var returns the default before consulting them -- so from_environment() produces a byte-identical AsyncConfig to before (verified by digesting every field on both trees). Only explicit opt-in/opt-out values change behaviour, and every value that already worked still does: "true"/"false"/"0" are unchanged, while "1"/"yes"/"on"/whitespace-padded values now mean what they say. logger.py's three AGENTFIELD_LOG_* flags use the same `== "true"` shape but all default to False, so they fail in the less harmful direction; left alone here to keep this to one seam.
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
There was a problem hiding this comment.
Reproduced this on main: AGENTFIELD_ASYNC_ENABLE_RESULT_CACHING=1 (and yes/on/padded true) really does switch caching off, and event_stream can't be turned on with 1/yes. Reverting async_config.py to main makes exactly the 33 new cases fail. ruff, mypy and the full run_pytest.sh suite (2512 passed) are green here. The changed lines are 100% covered when I measure async_config directly, since it isn't in the patch gate's --cov list, as you pointed out. One optional follow-up, not blocking: because the polarity is picked per field, a subclass that flips a default (say fallback_to_sync=False) would get True for an unrecognised value. A single parser that raises ValueError on unknown tokens would let get_env_var's existing fallback return cls()'s real default. Thanks for the careful write-up.
…ault (#1100) The two boolean parsers added in #1082 each bake in a polarity: _env_flag_default_on returns True for anything outside the falsey vocabulary and _env_flag_default_off returns False for anything outside the truthy one. That is correct for AsyncConfig's own field defaults, but it makes the fallback independent of the class -- a subclass that changes a default gets the parser's answer for an unrecognised string instead of its own: @DataClass class CachingOff(AsyncConfig): enable_result_caching: bool = False # AGENTFIELD_ASYNC_ENABLE_RESULT_CACHING=maybe CachingOff.from_environment().enable_result_caching # True, should be False Add one polarity-free parser, _env_flag, that raises ValueError outside the vocabulary, and use it for all five flags. get_env_var already catches (ValueError, TypeError) and returns the default_value it was handed, which from_environment() passes as config.<field> -- so the fallback now comes from the class, exactly as the float and int converters in the same function already do. This is the second half of #1089; the remaining strict parsers in logger.py, agent.py and agent_server.py are handled separately in #1090. It also matches the Go control plane, which has always parsed this way: config.applyBoolEnv leaves *target untouched and logs a warning when strconv.ParseBool rejects the string, and the handler tests pin parseBool(" YeS ") -> true, parseBool("0") -> false, parseBool("maybe") -> error. _env_flag_default_on and _env_flag_default_off are kept, now expressed in terms of _env_flag, for call sites that know their default statically -- #1090 imports _env_flag_default_off into logger.py for the default-off AGENTFIELD_LOG_* flags, so removing them would break an open PR. A test pins both wrappers' contract. AsyncConfig's own observable behaviour is unchanged: with no AGENTFIELD_ASYNC_* variable set the converters are never reached, and digesting every field of from_environment() on both trees gives the same SHA-256 (5960894a0c0bd6c817a09800eee26998e09625f7dfb23cd3d75ce1ee9088eb90). For a set variable, every value in the vocabulary parses as before and every value outside it still yields the field default -- the default just now comes from the class. Fixes the AsyncConfig half of #1089.
Summary
AsyncConfig.from_environment()reads all five boolean feature flags withlambda x: x.lower() == "true". Four of those fields default toTrue, so the only expressiblevalues were
trueandfalse— and a value that conventionally means "on" turns the feature off:AGENTFIELD_ASYNC_ENABLE_RESULT_CACHING=main1yesontruemaybe,2, ``So
AGENTFIELD_ASYNC_ENABLE_RESULT_CACHING=1, the most natural way to say "make sure caching is on",silently disables result caching — and
result_cache.pyshort-circuits at three separate points whenthe flag is false, so the cache is fully off, not degraded. The same inversion applies to
enable_async_execution(the master switch),enable_batch_pollingandfallback_to_sync. The fifthflag,
enable_event_stream, defaults toFalseand fails the other way:1/yes/oncannot turnSSE on at all.
This is the default path, not an opt-in helper. Both public constructors build their config this way
when the caller passes nothing —
agent.py:846andclient.py:182, bothasync_config or AsyncConfig.from_environment().test_client_default_async_config_uses_environmentalready pins that.
The lambda also breaks the documented contract. #714, which wired
from_environment()into the clientdefault, says it "only overrides fields when the corresponding env var is set (falling back to the
default on unparseable values)".
x.lower() == "true"never raises, soget_env_var'sexcept (ValueError, TypeError): return default_valuebranch is unreachable for booleans and a junkvalue overrides a default-
Truefield withFalse. The float and int converters in the same functiondo fall back correctly.
What changed
The package already has a vocabulary for this, split by which way the field defaults. Default-on
flags opt out with a falsey set:
log_writer._queue_enabled,logger._stdout_mirror_enabled,node_logs.logs_enabledandopenrouter_attribution.attribution_enabledall use("0", "false", "no", "off"). Default-off flags opt in with a truthy set:litellm_observability._TRUE_VALUESis{"1", "true", "yes", "on"}.async_config.pywas the onlymodule using
== "true"for flags that default toTrue, matching neither.So this adds two converters named for the polarity they serve and points the five flags at the right
one:
Picking the vocabulary by polarity fixes both halves at once: a value outside the set now yields the
field's default, so "fall back to the default on unparseable values" holds without the converter
having to raise.
.strip()is included because all four default-on precedents strip, and valuessourced from a shell or a YAML block routinely carry padding.
from_environment()'s docstring nowstates the vocabulary, since it previously documented only the prefix and two numeric examples.
Backward compatible.
get_env_varreturns the default before consulting the converter when thevariable is unset, so with no
AGENTFIELD_ASYNC_*set the new code is never reached. Digesting everyfield of
from_environment()on both trees with a clean environment gives the same SHA-256(
5960894a0c0bd6c817a09800eee26998e09625f7dfb23cd3d75ce1ee9088eb90) and the same boolean map. Everyvalue that already worked still does —
true/TRUE/false/FALSE/0/no/offare unchanged;1/yes/on, padded values and unparseable ones now behave as documented.Type of change
Test plan
76 parametrized tests added to
sdk/python/tests/test_async_config.py; all offline, no network orcredentials. On unmodified
mainthey give 33 failed, 49 passed, distributed exactly as the defectpredicts:
test_default_on_flag_accepts_conventional_truthy_values— 4 flags ×1/yes/on/true(the
true/TRUEcases already passed)test_default_on_flag_falls_back_to_default_when_unparseable— 4 flags ×maybe/``/2test_event_stream_opts_in_with_truthy_valuestest_boolean_env_flags_reach_the_client_default— the publicAgentFieldClient()pathTwo groups pass on
maintoo, and are there to pin what must not change:test_default_on_flag_still_opts_out(24 cases, the falsey vocabulary already worked) andtest_event_stream_stays_off_for_falsey_or_unparseable(9 cases). No existing test pinned the buggybehaviour — the six pre-existing tests use only
"false"and"true", which behave identicallybefore and after, so all six still pass.
Commands run locally (Windows, CPython 3.13.12 in an isolated venv,
pip install -e ".[dev]"plusruff==0.15.22at the versionsdk-python.ymlpins):cd sdk/python && python -m pytest tests/test_async_config.py --no-cov→ 82 passed(6 pre-existing + 76 new; 33 of the new ones fail before the source change)
cd sdk/python && python -m pytest --ignore=tests/test_agent_mesh.py --no-cov→9 failed, 2466 passed, 6 skipped with the fix, against 9 failed, 2390 passed, 6 skipped
on unmodified
main.2466 = 2390 + 76, and the sorted set of failing test ids isbyte-identical to
main, so nothing regressed. Those 9 are pre-existing Windows-host failures —'gbk' codec can't decodefrompathlib.read_text, andb'inline\r' != b'inline\n'CRLFassertions — that pass on CI's
ubuntu-latest.tests/test_agent_mesh.pyis excluded because ithangs on this host at ~8%, verified to hang identically on unmodified
main.cd sdk/python && python -m mypy --config-file mypy.ini agentfield/→ Success: no issues foundin 81 source files
cd sdk/python && python -m ruff check .→ All checks passedcd sdk/python && python -m ruff format --diff agentfield/async_config.py tests/test_async_config.py→ 2 files already formatted
Test coverage
coverage-baseline.json— not applicable, no source lines removedand no surface percentage can move (see below).
./scripts/coverage-summary.shlocally because it measures all five surfaces and this machinehas no Go, Bun, or Docker engine. Leaving this for CI rather than claiming it.
One thing worth flagging, because the gate will look odd:
agentfield.async_configis not in the--covwhitelist inpyproject.toml's pytestaddopts(which lists nine modules), andscripts/coverage-surface.shmeasuressdk-pythonby running barepytest, so this file neverappears in
sdk-python-coverage.xml.patch-coverage-gate.shrunsdiff-coveragainst that XML andcan only count lines the report knows about, so this PR will report
sdk-python | 0 | — | ➖ no changes. #1078 shows the mechanism: it touched 2 executable lines inrate_limiter.py(whitelisted) plus 48 lines of test code, and the gate reported exactly2 | 100.00%.So the patch gate passing here is not evidence that the new code is tested — the 76 tests above
are, and they exercise both converters through
from_environment()for all five flags across truthy,falsey, padded, unparseable and unset values. Adding
--cov=agentfield.async_configtoaddoptswouldlet the gate see this module, but that means editing
pyproject.toml; I kept it out of this PR tostay on one seam. Happy to send it separately if you want it.
Checklist
docs/CONTRIBUTING.mdanddocs/DEVELOPMENT.md.conventional-commits; the commit is not signed, as I have no signing key configured. Flagging
it rather than ticking the box.
contract this restores.
Related issues / PRs
Refs #621 (
[Python SDK] Inconsistent defaults for AsyncConfig and AIConfig) and #714, which madefrom_environment()the client default and documented the fallback-to-default behaviour this restores.Neither is about boolean value formats, so this isn't reversing a decision made there.
No open PR or branch covers it:
git log --all -- sdk/python/agentfield/async_config.pyshows fourcommits ever, none touching env-flag parsing, and across all 150 remote refs the file is either the
current
mainblob or one of two historical blobs — both of which still contain the samex.lower() == "true"lambda five times.Not touched here, but noting it since it is the same shape:
logger.py:112/115/117uses== "true"for
AGENTFIELD_LOG_PAYLOADS,AGENTFIELD_LOG_TRACKINGandAGENTFIELD_LOG_FIRE. All three defaultto
"false", so they fail in the less harmful direction —=1can't turn them on, but nothingdefault-on gets silently disabled. Separate seam, happy to do it as a follow-up if you'd like the
vocabulary unified across the package.