From acb105cf7d60258dee5e37b08ee8fff22e69f845 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 02:44:38 -0700 Subject: [PATCH] =?UTF-8?q?fix(config):=20route=20remaining=20boolean=20ke?= =?UTF-8?q?ys=20through=20=5Fconfig=5Fbool=20=E2=80=94=20string=20'false'?= =?UTF-8?q?=20no=20longer=20truthy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to microsoft/amplifier-module-provider-openai#74 (squash 8485663). That PR's read-only cross-check of this repo (HEAD 9916a68) found this module already has a correct boolean-parsing helper — AnthropicProvider._config_bool() (__init__.py:577-584) — used by six config keys, but FIVE boolean-ish keys bypassed it and read config with no coercion at all. The app-cli wizard writes boolean ConfigField answers as the STRING "true"/"false" (see the field_type="boolean" fields in get_info()). A plain self.config.get(key, default) returns that string unchanged, and every one of these keys is consumed in a truthiness context — any non-empty string, including the literal string "false", is truthy in Python. A user answering "false" in the wizard therefore got the feature turned ON. enable_prompt_caching is the live-reachable instance: it IS exposed as a field_type="boolean" ConfigField with string default "true" (__init__.py:975-982), so a wizard-driven "false" answer silently enabled prompt caching — and, post-#104, also fed the cache_stable_region_ttl_1h beta-header gate (if self.enable_prompt_caching: at __init__.py:812), which then misread too. Audit table (full re-audit of every self.config.get() call in the constructor, not just the 5 keys named in the task): | key | site (pre-fix) | was broken? | fixed how | |-------------------------|----------------|-------------|--------------------------------| | raw | :661 | YES — no coercion | routed through _config_bool() | | use_streaming | :748 | YES — no coercion | routed through _config_bool() | | filtered | :749-751 | YES — no coercion | routed through _config_bool() | | enable_prompt_caching | :752 | YES — no coercion; wizard-exposed boolean ConfigField, LIVE-REACHABLE | routed through _config_bool() | | enable_web_search | :753-755 | YES — no coercion | routed through _config_bool() | | retry_jitter | :676 | already safe | uses _config_bool() (pre-existing) | | fallback_on_overload | :691-693 | already safe | uses _config_bool() (pre-existing) | | enable_1m_context | :703-705 | already safe | uses _config_bool() (pre-existing) | | persist_fallback_state | :712-714 | already safe | uses _config_bool() (pre-existing) | | refusal_fallback_enabled| :720-722 | already safe | uses _config_bool() (pre-existing) | | cache_stable_region_ttl_1h | :773-775 | already safe | uses _config_bool() (pre-existing) | No other uncoerced boolean-ish config key exists in the constructor — every remaining self.config.get() call reads a numeric, string, or choice value (max_tokens, temperature, timeout, model names, thinking_type, speed, etc.), not a boolean. Fix: each of the five keys is now read as `self._config_bool(self.config.get(key, default))`, mirroring the exact call shape already used by the six safe keys. This module's _config_bool() coerces (does not fail loud on garbage — anything outside 1/true/yes/on resolves to False); no new helper was introduced, matching the module's own existing convention exactly, per the task's own guidance to not invent a new helper. Tests: tests/test_config_bool_parsing.py — 21 tests covering, per affected key: string "false" -> False, string "true" -> True, real bool passthrough, absent -> documented default; plus one integration-flavored assertion for the live-reachable key (enable_prompt_caching="false" as a string -> zero cache_control blocks in a built request). Fail-before/pass-after proof (git stash of only the source fix, keeping the new test file in place): 11 failed / 10 passed against pre-fix main (the 10 passes are the real-bool-passthrough and absent-default cases, which were never broken). Restoring the fix: all 21 pass. Full suite: 695 passed (baseline 674 passed confirmed via a clean run before touching anything, post-#103/#104, + 21 new tests = 695 exactly). No regressions. python_check on touched files: amplifier_module_provider_anthropic/__init__.py carries 15 pre-existing pyright errors + 18 pre-existing ruff-lint/stub warnings (SDK Optional-attribute narrowing, a pre-existing unsorted __all__/import block, blind-exception lint nits, a TODO stub comment) — confirmed identical (15 errors / 18 warnings, same codes and same line-number deltas as this diff's own +6 net lines) before and after this change via git stash. `ruff format`/`ruff check` show zero diff needed on this diff's own lines. The new test file is `ruff format`/`ruff check` clean; its two pyright import-resolution errors are the same isolated-file false positive every existing test file in this repo also reports when checked in isolation (verified against tests/test_prompt_cache_breakpoints.py, an unmodified pre-existing file, which reports the identical "AnthropicProvider is unknown import symbol" / "tests._helpers could not be resolved" pair) — a tool-environment artifact of checking a test file outside the project's own pytest rootdir/venv resolution, not a real defect. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- .../__init__.py | 18 +- tests/test_config_bool_parsing.py | 175 ++++++++++++++++++ 2 files changed, 186 insertions(+), 7 deletions(-) create mode 100644 tests/test_config_bool_parsing.py diff --git a/amplifier_module_provider_anthropic/__init__.py b/amplifier_module_provider_anthropic/__init__.py index cc70990..097d954 100644 --- a/amplifier_module_provider_anthropic/__init__.py +++ b/amplifier_module_provider_anthropic/__init__.py @@ -658,7 +658,9 @@ def __init__( ) self.temperature = self.config.get("temperature", 0.7) self.priority = self.config.get("priority", 100) # Store priority for selection - self.raw = self.config.get("raw", False) # Include raw payload in events + self.raw = self._config_bool( + self.config.get("raw", False) + ) # Include raw payload in events self.timeout = self.config.get( "timeout", 600.0 ) # API timeout in seconds (default 10 minutes) @@ -745,13 +747,15 @@ def __init__( # Use streaming API by default to support large context windows (Anthropic requires streaming # for operations that may take > 10 minutes, e.g. with 300k+ token contexts) - self.use_streaming = self.config.get("use_streaming", True) - self.filtered = self.config.get( - "filtered", True + self.use_streaming = self._config_bool(self.config.get("use_streaming", True)) + self.filtered = self._config_bool( + self.config.get("filtered", True) ) # Filter to curated model list by default - self.enable_prompt_caching = self.config.get("enable_prompt_caching", True) - self.enable_web_search = self.config.get( - "enable_web_search", False + self.enable_prompt_caching = self._config_bool( + self.config.get("enable_prompt_caching", True) + ) + self.enable_web_search = self._config_bool( + self.config.get("enable_web_search", False) ) # Enable native web search tool # Extended (1-hour) TTL for the stable system/tools cache breakpoints. diff --git a/tests/test_config_bool_parsing.py b/tests/test_config_bool_parsing.py new file mode 100644 index 0000000..ea643b2 --- /dev/null +++ b/tests/test_config_bool_parsing.py @@ -0,0 +1,175 @@ +"""Tests for boolean config parsing on the keys that bypassed `_config_bool()`. + +Follow-up to `microsoft/amplifier-module-provider-openai#74`. That PR fixed the +same anti-pattern in the openai provider and, as a read-only cross-check +(HEAD `9916a68`), found this module already has a correct helper -- +`AnthropicProvider._config_bool()` (`__init__.py:577-584`) -- used by six +config keys, but FIVE boolean-ish keys were read straight off +`self.config.get(key, default)` with no coercion at all: + + raw, use_streaming, filtered, enable_prompt_caching, enable_web_search + +The app-cli wizard writes boolean `ConfigField` answers as the STRING +``"true"``/``"false"`` (see the ``field_type="boolean"`` fields in +``get_info()``), and a plain ``self.config.get(key, default)`` with no +coercion returns that string unchanged. Every one of these keys is then used +in a truthiness context (``if self.filtered:``, ``if self.enable_web_search:``, +etc.), and any non-empty string -- including the literal string ``"false"`` +-- is truthy in Python. A user answering "false" in the wizard therefore gets +the feature turned ON. + +``enable_prompt_caching`` is the live-reachable instance: it IS exposed as a +``field_type="boolean"`` `ConfigField` with string default ``"true"`` +(`__init__.py:975-982`), so a wizard-driven ``"false"`` answer silently +enables prompt caching -- and, post-#104, also feeds the +``cache_stable_region_ttl_1h`` beta-header gate +(``if self.enable_prompt_caching:`` at `__init__.py:813`), which then +misreads too. + +Each test below is written so it FAILS on pre-fix `main` (string ``"false"`` +resolves truthy) and PASSES once the key is routed through the existing +``_config_bool()`` helper -- verified via `git stash` (see the PR body for +the exact before/after run). +""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock + +import pytest +from amplifier_core.message_models import ChatRequest, Message + +from amplifier_module_provider_anthropic import AnthropicProvider +from tests._helpers import DummyResponse + +# --------------------------------------------------------------------------- +# Per-key parametrized coverage +# --------------------------------------------------------------------------- + +# (config key, attribute name on the provider instance, default value) +AFFECTED_KEYS = [ + ("raw", "raw", False), + ("use_streaming", "use_streaming", True), + ("filtered", "filtered", True), + ("enable_prompt_caching", "enable_prompt_caching", True), + ("enable_web_search", "enable_web_search", False), +] + + +def _make_provider(config: dict) -> AnthropicProvider: + return AnthropicProvider(api_key="test-key", config=config) + + +@pytest.mark.parametrize("key,attr,default", AFFECTED_KEYS) +def test_string_false_resolves_to_real_false(key: str, attr: str, default: bool): + """The exact bug: string "false" must resolve to boolean False. + + Fails on pre-fix main because `bool("false")` / bare truthiness on the + string `"false"` is `True`. + """ + provider = _make_provider({key: "false"}) + assert getattr(provider, attr) is False, ( + f"{key}='false' (string) resolved truthy -- the wizard-writes-strings " + "bug is present" + ) + + +@pytest.mark.parametrize("key,attr,default", AFFECTED_KEYS) +def test_string_true_resolves_to_real_true(key: str, attr: str, default: bool): + """String "true" must resolve to boolean True (sanity: not just always-False).""" + provider = _make_provider({key: "true"}) + assert getattr(provider, attr) is True + + +@pytest.mark.parametrize("key,attr,default", AFFECTED_KEYS) +def test_real_bool_passthrough(key: str, attr: str, default: bool): + """Real booleans (already-correct config, e.g. from a Python caller or a + YAML `true`/`false` literal parsed by PyYAML) must pass through unchanged.""" + provider_true = _make_provider({key: True}) + provider_false = _make_provider({key: False}) + assert getattr(provider_true, attr) is True + assert getattr(provider_false, attr) is False + + +@pytest.mark.parametrize("key,attr,default", AFFECTED_KEYS) +def test_absent_key_uses_documented_default(key: str, attr: str, default: bool): + """An absent key must fall back to the key's documented default.""" + provider = _make_provider({}) + assert getattr(provider, attr) is default + + +# --------------------------------------------------------------------------- +# Integration-flavored assertion for the live-reachable key: +# enable_prompt_caching="false" (string, exactly what the wizard writes) must +# result in NO cache_control blocks anywhere in a built request -- mirrors +# `test_prompt_caching_disabled_places_no_breakpoints_at_all` in +# test_prompt_cache_breakpoints.py, but drives the config through the +# wizard's actual string shape instead of a real Python bool. +# --------------------------------------------------------------------------- + + +def _count_cache_control_blocks(params: dict) -> int: + count = 0 + for block in params.get("system") or []: + if isinstance(block, dict) and "cache_control" in block: + count += 1 + for tool in params.get("tools") or []: + if isinstance(tool, dict) and "cache_control" in tool: + count += 1 + for msg in params.get("messages") or []: + content = msg.get("content") + if isinstance(content, list): + for block in content: + if isinstance(block, dict) and "cache_control" in block: + count += 1 + return count + + +def _capture_params(provider: AnthropicProvider) -> dict: + captured: dict = {} + + async def _fake_create(**params): + captured.update(params) + raw = MagicMock() + raw.parse = AsyncMock(return_value=DummyResponse()) + raw.headers = {} + return raw + + provider.client.messages.with_raw_response.create = AsyncMock( + side_effect=_fake_create + ) + return captured + + +def test_wizard_string_false_disables_prompt_caching_end_to_end(): + """The live-reachable regression: `enable_prompt_caching: "false"` (a + string, exactly what the app-cli wizard writes for a boolean ConfigField + answer) must produce a request with ZERO cache_control blocks -- not the + inverted "caching stays on" behavior the pre-fix truthiness bug produces. + """ + provider = AnthropicProvider( + api_key="test-key", + config={"use_streaming": False, "enable_prompt_caching": "false"}, + ) + params = _capture_params(provider) + + messages = [ + Message(role="system", content="System prompt."), + Message(role="user", content="question"), + Message(role="assistant", content="answer"), + ] + request = ChatRequest(messages=messages) + + async def _complete_and_close() -> None: + await provider.complete(request) + await provider.close() + + asyncio.run(_complete_and_close()) + + assert provider.enable_prompt_caching is False, ( + "enable_prompt_caching='false' (string) did not resolve to False -- " + "the wizard-writes-strings bug is present" + ) + assert _count_cache_control_blocks(params) == 0, ( + "enable_prompt_caching='false' (string) still placed cache_control " + f"blocks in the request: {params}" + )