fix(config): route remaining boolean keys through _config_bool — string 'false' no longer truthy - #105
Merged
Conversation
…ng 'false' no longer truthy 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>
Collaborator
Author
|
Self-authored fix, merging at user direction under the documented maintainer admin-merge pattern (see microsoft/amplifier-module-provider-openai#74 for precedent). All required checks are green: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Follow-up to
microsoft/amplifier-module-provider-openai#74(squash8485663).That PR's read-only cross-check of this repo (HEAD
9916a68) found thismodule already has a correct boolean-parsing helper —
AnthropicProvider._config_bool()(__init__.py:577-584) — used by sixconfig keys, but five boolean-ish keys bypassed it and read config with no
coercion at all.
The app-cli wizard writes boolean
ConfigFieldanswers as the STRING"true"/"false"(see thefield_type="boolean"fields inget_info()).A plain
self.config.get(key, default)returns that string unchanged, andevery one of these keys is consumed in a truthiness context — any
non-empty string, including the literal string
"false", is truthy inPython. A user answering "false" in the wizard therefore got the feature
turned ON.
enable_prompt_cachingis the live-reachable instance: it IS exposedas a
field_type="boolean"ConfigFieldwith string default"true"(
__init__.py:975-982), so a wizard-driven"false"answer silentlyenabled prompt caching — and, post-#104, also fed the
cache_stable_region_ttl_1hbeta-header gate(
if self.enable_prompt_caching:at__init__.py:812), which thenmisread too.
Audit table
Full re-audit of every
self.config.get()call in the constructor, notjust the five keys named going in:
raw:661_config_bool()use_streaming:748_config_bool()filtered:749-751_config_bool()enable_prompt_caching:752ConfigField, LIVE-REACHABLE_config_bool()enable_web_search:753-755_config_bool()retry_jitter:676_config_bool()(pre-existing)fallback_on_overload:691-693_config_bool()(pre-existing)enable_1m_context:703-705_config_bool()(pre-existing)persist_fallback_state:712-714_config_bool()(pre-existing)refusal_fallback_enabled:720-722_config_bool()(pre-existing)cache_stable_region_ttl_1h:773-775_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, orchoice value (
max_tokens,temperature,timeout, model names,thinking_type,speed, etc.), never a boolean.The fix
Each of the five keys is now read as
self._config_bool(self.config.get(key, default)), mirroring the exactcall shape already used by the six safe keys. This module's
_config_bool()coerces (does not fail loud on garbage — anythingoutside
1/true/yes/onresolves toFalse); no new helper wasintroduced, matching the module's own existing convention exactly.
Tests
tests/test_config_bool_parsing.py— 21 tests:"false"→False, string"true"→True,real bool passthrough, absent → documented default
enable_prompt_caching="false"(string) → zerocache_controlblocksin a built request
Fail-before/pass-after proof: stashed just the source fix (kept the
new test file) and ran the new tests against pre-fix
main— 11failed / 10 passed (the 10 passes are the real-bool-passthrough and
absent-default cases, which were never broken — real bools and defaults
were always the correct type). Restored the fix — all 21 pass.
Full suite:
695 passed— baseline was674 passed(verified via aclean run before touching anything, post-#103/#104), plus the 21 new
tests =
695exactly. No regressions.python_checkon touched files:amplifier_module_provider_anthropic/__init__.pycarries 15 pre-existingpyright errors + 18 pre-existing ruff-lint/stub warnings (SDK
Optional-attribute narrowing, an unsorted
__all__/import block, blind-exception lint nits, a TODO stub comment) — confirmed identical (15
errors / 18 warnings, same codes, line numbers shifted only by this
diff's own +6 net lines) before and after this change via
git stash.ruff format/ruff checkshow zero diff needed on this diff's ownlines. The new test file is
ruff format/ruff checkclean; its twopyright import-resolution errors are the same isolated-file false
positive every existing test file in this repo also reports when checked
outside the project's pytest rootdir/venv (verified against the
unmodified
tests/test_prompt_cache_breakpoints.py, which reports theidentical
"AnthropicProvider" is unknown import symbol/"tests._helpers" could not be resolvedpair) — a tool-environmentartifact, not a real defect.
Precedent
microsoft/amplifier-module-provider-openai#74(squash8485663) fixedthe identical anti-pattern in the openai provider and did the read-only
cross-check that found this repo's gap. This PR closes that gap.
🤖 Generated with Amplifier
Co-Authored-By: Amplifier 240397093+microsoft-amplifier@users.noreply.github.com