fix: coerce use_streaming string config, warn on unknown config keys, tighten ConfigField prompts

amplifier_module_provider_github_copilot/__init__.py

@@ -112,6 +112,7 @@ def _parse_sdk_version(version_str: str) -> _Version:
 
 # E402: These imports are intentionally after SDK check - we verify SDK
 # installation before importing modules that depend on it (Two-Medium Architecture).
+import difflib  # noqa: E402
 import logging  # noqa: E402
 from collections.abc import Awaitable, Callable  # noqa: E402
 from typing import Any, NoReturn  # noqa: E402
@@ -299,6 +300,93 @@ async def _release_shared_client() -> None:
             )
 
 
+# Contract: config-key hygiene — the exact set of config keys this provider
+# reads or otherwise recognizes as legitimate. Keep in sync with every
+# `self.config.get(...)` (GitHubCopilotProvider.__init__, provider.py) and
+# `config.get(...)` (_build_retry_config, provider.py) call site.
+# Warn-only sweep at mount time (see _warn_unknown_config_keys below) --
+# NEVER raises, so a typo'd or stale key does not block mount().
+_KNOWN_CONFIG_KEYS: frozenset[str] = frozenset(
+    {
+        # Read directly by GitHubCopilotProvider.__init__ (provider.py).
+        "github_token",
+        "default_model",
+        "raw",
+        "enable_long_context",
+        "reasoning_effort",
+        "use_streaming",
+        # Read directly by _build_retry_config (provider.py).
+        "max_retries",
+        "min_retry_delay",
+        "max_retry_delay",
+        "retry_jitter",
+        "overloaded_delay_multiplier",
+        # Not read by this module at all, but a legitimate, LIVE-consumed key:
+        # read straight off this same config dict by the orchestrator's own
+        # provider-selection logic (loop-streaming's _select_provider).
+        # Flagging it as "unknown" would tell an operator to delete a
+        # setting that is actively working.
+        "priority",
+        # Not read by this module; reserved by amplifier-app-cli's own
+        # session-config passthrough schema.
+        "extra_request_params",
+    }
+)
+
+# Keys this provider does NOT recognize but that must never get the generic
+# did-you-mean treatment -- either because a "correction" would be nonsensical
+# or because implying a typo would be dishonest.
+# "debug": present in ~9 of the maintainer's own test fixture configs;
+# genuinely unread anywhere in this provider (no `config.get("debug", ...)`
+# call exists). Not added to `_KNOWN_CONFIG_KEYS` above (it is not a key this
+# provider reads) -- instead given an honest, targeted message so test output
+# (and any real caller's logs) stays clean and truthful rather than guessing.
+_TARGETED_UNKNOWN_KEY_MESSAGES: dict[str, str] = {
+    "debug": "not read by this provider",
+}
+
+
+def _warn_unknown_config_keys(config: dict[str, Any]) -> None:
+    """Warn (never raise) about config keys this provider does not recognize.
+
+    A typo'd or stale option (e.g. ``us_streaming`` instead of
+    ``use_streaming``) is silently inert today -- the config author gets no
+    signal their setting had no effect. This surfaces it loudly at mount
+    time: one combined warning naming every offender, each with a
+    nearest-valid-key suggestion (via `difflib`) when one exists, EXCEPT for
+    keys in `_TARGETED_UNKNOWN_KEY_MESSAGES`, which get their own honest
+    message instead of a did-you-mean guess.
+
+    Must stay quiet on every legitimate config: `_KNOWN_CONFIG_KEYS` is the
+    full set of keys this provider (and its live orchestrator/app-cli
+    collaborators) recognize. No false positives are acceptable here --
+    when in doubt, a key belongs in that set, not flagged.
+
+    Args:
+        config: The provider's resolved config dict, as passed to mount().
+    """
+    unknown = sorted(set(config) - _KNOWN_CONFIG_KEYS)
+    if not unknown:
+        return
+    described: list[str] = []
+    for key in unknown:
+        targeted = _TARGETED_UNKNOWN_KEY_MESSAGES.get(key)
+        if targeted is not None:
+            described.append(f"{key!r} ({targeted})")
+            continue
+        match = difflib.get_close_matches(key, _KNOWN_CONFIG_KEYS, n=1)
+        if match:
+            described.append(f"{key!r} (did you mean {match[0]!r}?)")
+        else:
+            described.append(repr(key))
+    logging.getLogger(__name__).warning(
+        "[MOUNT] Unrecognized config key(s) for provider-github-copilot: %s. "
+        "These have no effect -- likely a typo or a stale/removed option. "
+        "See the module README for the full list of accepted config keys.",
+        ", ".join(described),
+    )
+
+
 def _apply_config_github_token(config: dict[str, Any]) -> None:
     """Promote an explicit config-provided github_token into the environment.
 
@@ -398,6 +486,13 @@ async def mount(
 
     config = config or {}
 
+    # Warn (never raise) about config keys this provider does not recognize.
+    # Guarded: a logging failure must never block mount().
+    try:
+        _warn_unknown_config_keys(config)
+    except Exception:  # pragma: no cover  # diagnostic only — never propagate out of mount()
+        pass
+
     # Promote an explicit config-provided github_token into the environment
     # BEFORE auth-source resolution/logging below, so a hand-written config
     # value is honored the same way a wizard-collected one already is (see

amplifier_module_provider_github_copilot/provider.py

@@ -508,6 +508,11 @@ def __init__(
         self._enable_long_context: bool = _parse_raw_flag(
             self.config.get("enable_long_context", False)
         )
+        # Reuse the bool parser (see _parse_raw_flag) — avoids the same
+        # bool("false")==True footgun for use_streaming; mirrors self._raw
+        # and self._enable_long_context. Default stays True (unchanged).
+        # Contract: provider-streaming-contract.md — use_streaming config
+        self._use_streaming: bool = _parse_raw_flag(self.config.get("use_streaming", True))
         # Parse retry config once at init — allows per-instance user overrides
         self._retry_config: RetryConfig = _build_retry_config(self.config, load_retry_config())
         # Track pending streaming emit tasks for cleanup
@@ -597,24 +602,31 @@ def get_info(self) -> ProviderInfo:
                     id="github_token",
                     display_name="GitHub Token",
                     field_type="secret",
-                    prompt="Enter your GitHub token (or Copilot agent token)",
+                    # Prompt-text tightening only -- id/field_type/env_var/required unchanged.
+                    prompt="GitHub token (or Copilot agent token)",
                     env_var="GITHUB_TOKEN",
                     required=True,
                 ),
                 ConfigField(
                     id="enable_long_context",
                     display_name="Long context tier by default",
                     field_type="boolean",
-                    prompt="Default to the long-context tier when the model supports it",
+                    # Prompt-text tightening only -- id/field_type/default/required/
+                    # requires_model unchanged.
+                    prompt="Use the long-context tier by default?",
                     required=False,
                     default="false",
                     requires_model=True,
                 ),
+                # Contract: provider-protocol:get_info:MUST:6 -- MUST appear immediately
+                # after enable_long_context; choices list MUST NOT change.
                 ConfigField(
                     id="reasoning_effort",
                     display_name="Default reasoning effort",
                     field_type="choice",
-                    prompt="Select the default reasoning effort for supported models",
+                    # Prompt-text tightening only -- id/field_type/choices/default/
+                    # required/requires_model unchanged.
+                    prompt="Default reasoning effort",
                     choices=["model default", *REASONING_EFFORT_LEVELS],
                     required=False,
                     default="model default",
@@ -788,9 +800,10 @@ async def complete(
 
             # Emit llm:request event (contract: observability:Events:MUST:2)
             # Contract: provider-streaming-contract.md — use_streaming config + metadata override
-            use_streaming: bool = self.config.get("use_streaming", True)
+            # self._use_streaming parsed once in __init__ (see _parse_raw_flag) —
+            # avoids the bool("false")==True footgun for a string config value.
             _meta = getattr(request, "metadata", None)
-            _use_streaming: bool = use_streaming
+            _use_streaming: bool = self._use_streaming
             if isinstance(_meta, dict) and _meta.get("stream") is False:
                 # Identity check per contract: `is False` not `==False`
                 _use_streaming = False

tests/test_config_field_conformance.py

@@ -91,4 +91,6 @@ def test_github_token_field_display_and_prompt(self) -> None:
         token_field = token_fields[0]
 
         assert token_field.display_name == "GitHub Token"
-        assert token_field.prompt == "Enter your GitHub token (or Copilot agent token)"
+        # Prompt text tightened (see provider.py get_info()) -- id/field_type/
+        # env_var/required are unchanged and covered by the tests above.
+        assert token_field.prompt == "GitHub token (or Copilot agent token)"

tests/test_provider_streaming_contract.py

@@ -711,6 +711,43 @@ def test_use_streaming_can_be_disabled(self) -> None:
         provider = GitHubCopilotProvider(config={"use_streaming": False})
         assert provider.config.get("use_streaming", True) is False
 
+    def test_use_streaming_string_false_is_coerced(self) -> None:
+        """use_streaming="false" (string, e.g. from YAML/env) must NOT be truthy.
+
+        Regression test for the bool("false") == True footgun: a raw
+        ``self.config.get("use_streaming", True)`` treats any non-empty
+        string as truthy, so a config-provided string "false" was silently
+        treated as "streaming enabled". Parsed once at __init__ via the
+        same ``_parse_raw_flag`` helper used for ``raw`` and
+        ``enable_long_context`` (provider.py:505,509).
+
+        Contract: provider-streaming-contract.md -- use_streaming config.
+        """
+        from amplifier_module_provider_github_copilot.provider import GitHubCopilotProvider
+
+        provider = GitHubCopilotProvider(config={"use_streaming": "false"})
+        assert provider._use_streaming is False
+
+    def test_use_streaming_string_true_is_coerced(self) -> None:
+        """use_streaming="true" (string) parses to bool True (not just truthy)."""
+        from amplifier_module_provider_github_copilot.provider import GitHubCopilotProvider
+
+        provider = GitHubCopilotProvider(config={"use_streaming": "true"})
+        assert provider._use_streaming is True
+
+    def test_use_streaming_bool_still_works(self) -> None:
+        """use_streaming=True/False (native bool) continues to work unchanged."""
+        from amplifier_module_provider_github_copilot.provider import GitHubCopilotProvider
+
+        assert GitHubCopilotProvider(config={"use_streaming": True})._use_streaming is True
+        assert GitHubCopilotProvider(config={"use_streaming": False})._use_streaming is False
+
+    def test_use_streaming_default_when_absent(self) -> None:
+        """use_streaming absent from config defaults to True (unchanged default)."""
+        from amplifier_module_provider_github_copilot.provider import GitHubCopilotProvider
+
+        assert GitHubCopilotProvider(config={})._use_streaming is True
+
     def test_stream_false_metadata_override_logic(self) -> None:
         """metadata={'stream': False} uses identity check (is False, not ==False)."""
         use_streaming = True

tests/test_unknown_config_keys.py

@@ -0,0 +1,118 @@
+"""Tests for the warn-only unknown-config-key sweep at mount time.
+
+Contract: config-key hygiene (see `_KNOWN_CONFIG_KEYS` / `_warn_unknown_config_keys`
+in amplifier_module_provider_github_copilot/__init__.py).
+
+These tests exercise `_warn_unknown_config_keys` directly (pure function, no SDK
+interaction) rather than the full `mount()` coroutine, since mount() requires a
+live coordinator and SDK client plumbing unrelated to this concern.
+"""
+
+from __future__ import annotations
+
+import logging
+
+import pytest
+
+from amplifier_module_provider_github_copilot import (
+    _KNOWN_CONFIG_KEYS,
+    _warn_unknown_config_keys,
+)
+
+# All 13 keys the deep-read confirmed: 11 read directly by this module's own
+# `self.config.get(...)` / `config.get(...)` call sites, plus 2 keys this
+# module never reads itself but that are live, legitimately-consumed keys
+# read by other collaborators off this same config dict (`priority` by the
+# loop-streaming orchestrator's provider selection; `extra_request_params`
+# reserved by app-cli).
+_EXPECTED_KEYS = frozenset(
+    {
+        "github_token",
+        "default_model",
+        "raw",
+        "enable_long_context",
+        "reasoning_effort",
+        "use_streaming",
+        "max_retries",
+        "min_retry_delay",
+        "max_retry_delay",
+        "retry_jitter",
+        "overloaded_delay_multiplier",
+        "priority",
+        "extra_request_params",
+    }
+)
+
+
+class TestKnownConfigKeysAllowlist:
+    """The allowlist itself must be exactly the 13 keys the deep-read confirmed."""
+
+    def test_known_config_keys_is_exactly_the_expected_set(self) -> None:
+        assert _KNOWN_CONFIG_KEYS == _EXPECTED_KEYS
+        assert len(_KNOWN_CONFIG_KEYS) == 13
+
+
+class TestWarnUnknownConfigKeys:
+    """`_warn_unknown_config_keys` warns, never raises, and stays quiet on legit config."""
+
+    def test_all_known_keys_produce_no_warning(self, caplog: pytest.LogCaptureFixture) -> None:
+        config = dict.fromkeys(_KNOWN_CONFIG_KEYS, "x")
+        with caplog.at_level(logging.WARNING):
+            _warn_unknown_config_keys(config)
+        assert caplog.records == []
+
+    def test_empty_config_produces_no_warning(self, caplog: pytest.LogCaptureFixture) -> None:
+        with caplog.at_level(logging.WARNING):
+            _warn_unknown_config_keys({})
+        assert caplog.records == []
+
+    def test_typo_key_gets_did_you_mean_suggestion(self, caplog: pytest.LogCaptureFixture) -> None:
+        with caplog.at_level(logging.WARNING):
+            _warn_unknown_config_keys({"us_streaming": True})
+        assert len(caplog.records) == 1
+        message = caplog.records[0].getMessage()
+        assert "us_streaming" in message
+        assert "did you mean 'use_streaming'" in message
+
+    def test_unrecognizable_key_gets_bare_mention_no_suggestion(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        with caplog.at_level(logging.WARNING):
+            _warn_unknown_config_keys({"totally_unrelated_xyz": True})
+        assert len(caplog.records) == 1
+        message = caplog.records[0].getMessage()
+        assert "'totally_unrelated_xyz'" in message
+        assert "did you mean" not in message
+
+    def test_debug_key_gets_targeted_message_not_did_you_mean(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        """The maintainer's own fixtures pass `debug` in ~9 configs; it is
+        genuinely unread. It must still warn (honest signal), but with a
+        targeted message rather than a nonsensical did-you-mean guess.
+        """
+        with caplog.at_level(logging.WARNING):
+            _warn_unknown_config_keys({"debug": False})
+        assert len(caplog.records) == 1
+        message = caplog.records[0].getMessage()
+        assert "'debug'" in message
+        assert "not read by this provider" in message
+        assert "did you mean" not in message
+
+    def test_multiple_unknown_keys_combined_into_one_warning(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        with caplog.at_level(logging.WARNING):
+            _warn_unknown_config_keys({"debug": False, "us_streaming": True, "model": "x"})
+        assert len(caplog.records) == 1
+        message = caplog.records[0].getMessage()
+        assert "'debug'" in message
+        assert "us_streaming" in message
+        # "model" has no close match in _KNOWN_CONFIG_KEYS (closest is default_model,
+        # difflib may or may not match depending on similarity ratio) -- just assert
+        # it's named at all.
+        assert "'model'" in message
+
+    def test_never_raises_on_non_string_keys_or_odd_values(self) -> None:
+        """Defensive: sweep must not raise even with an unusual config shape."""
+        _warn_unknown_config_keys({"debug": None, "": "", "priority": 5})