diff --git a/amplifier_module_provider_github_copilot/__init__.py b/amplifier_module_provider_github_copilot/__init__.py index 372de26..c471d09 100644 --- a/amplifier_module_provider_github_copilot/__init__.py +++ b/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 diff --git a/amplifier_module_provider_github_copilot/provider.py b/amplifier_module_provider_github_copilot/provider.py index f343bab..a3ac85f 100644 --- a/amplifier_module_provider_github_copilot/provider.py +++ b/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,7 +602,8 @@ 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, ), @@ -605,16 +611,22 @@ def get_info(self) -> ProviderInfo: 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 diff --git a/tests/test_config_field_conformance.py b/tests/test_config_field_conformance.py index d73e4b6..ebbfcdc 100644 --- a/tests/test_config_field_conformance.py +++ b/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)" diff --git a/tests/test_provider_streaming_contract.py b/tests/test_provider_streaming_contract.py index 2ef6a07..8d43061 100644 --- a/tests/test_provider_streaming_contract.py +++ b/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 diff --git a/tests/test_unknown_config_keys.py b/tests/test_unknown_config_keys.py new file mode 100644 index 0000000..c3aa210 --- /dev/null +++ b/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})