Skip to content

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

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/use-streaming-coercion-unknown-config-sweep
Aug 29, 2026
Merged

fix: coerce use_streaming string config, warn on unknown config keys, tighten ConfigField prompts#80
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/use-streaming-coercion-unknown-config-sweep

Conversation

@bkrabach

Copy link
Copy Markdown
Contributor

Summary

Three conservative, contract-respecting fixes surfaced by a deep-read of this
provider's config-handling code paths. Each follows a pattern already
established elsewhere in this module; nothing here adds new ConfigFields,
touches session_config, the client singleton, or packaged YAML defaults.

1. use_streaming string coercion (provider.py:791 -> __init__)

self.config.get("use_streaming", True) was read raw at call time, so a
config-provided string "false" was truthy (bool("false") is True) and
silently kept streaming enabled -- the same bool("false")==True footgun
this module already guards against for raw and enable_long_context via
_parse_raw_flag (provider.py:173-191). Fixed by parsing once in __init__
as self._use_streaming, reusing _parse_raw_flag verbatim -- no new
helper, matching the exact pattern already used for self._raw (:505) and
self._enable_long_context (:509).

session_config["streaming"] stays hard-pinned True in
sdk_adapter/client.py:618 (deny-destroy contract) -- untouched. This flag
only gates Amplifier-side llm:stream_* event emission.

2. Warn-only unknown-config-key sweep at mount()

A typo'd or stale config key (e.g. us_streaming) was silently inert with
no signal to the operator. Added _warn_unknown_config_keys, called from
mount() guarded in a try/except (matches every other best-effort call in
mount() -- never raises, never blocks mount).

The allowlist (_KNOWN_CONFIG_KEYS) is exactly the 13 keys this provider (or
a live collaborator) recognizes:

  • 11 read directly via self.config.get(...) / config.get(...) in
    provider.py: 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: not read by this module, but read live off this same
    config dict by the orchestrator's own provider-selection logic
    (loop-streaming's _select_provider). Deliberately included so this sweep
    never tells an operator to delete a setting that is actively working.
  • extra_request_params: not read by this module; reserved by
    amplifier-app-cli's own session-config passthrough schema.

Unrecognized keys get a difflib.get_close_matches did-you-mean suggestion
when one exists. The literal key debug -- present in ~9 of the
maintainer's own test fixture configs, genuinely unread anywhere in this
provider -- gets an honest targeted message ("not read by this provider")
instead of a nonsensical did-you-mean guess, so both test output and any
real caller's logs stay truthful.

3. ConfigField prompt-text tightening (get_info())

Shortened the three existing ConfigField prompts:

  • github_token: "Enter your GitHub token (or Copilot agent token)" ->
    "GitHub token (or Copilot agent token)"
  • enable_long_context: "Default to the long-context tier when the model
    supports it" -> "Use the long-context tier by default?"
  • reasoning_effort: "Select the default reasoning effort for supported
    models" -> "Default reasoning effort"

No id, field_type, default, required/requires_model flag, choices,
or field order changed. get_info:MUST:6 (exact reasoning_effort choices
list, position immediately after enable_long_context) is preserved
verbatim. Updated the one assertion in test_config_field_conformance.py
that pinned the old github_token prompt text so it stays in sync with the
intentional change.

Deliberately NOT done (out of scope for this PR)

  • No new ConfigFields added -- provider-protocol.md:112-128 is a closed
    list, and the app-cli model picker already collects the model.
  • No extra_request_params ConfigField -- deny-destroy.md:157 forbids a
    session-config knob becoming a YAML setting ("NEVER configurable").
  • No mount-time model/effort heuristics.
  • No changes to session_config, the client singleton, or packaged YAML
    defaults (config/_models.py).

(A separate upstream issue covers the packaged default model
claude-opus-4.5 no longer existing in live catalogs -- deliberately not
fixed here since it's a policy/data decision for the maintainer, not a code
defect. See linked issue.)

Testing

  • Fail-before proof: ran a standalone repro plus the 4 new regression
    tests in TestUseStreamingConfig against unmodified main first --
    all 4 failed with
    AttributeError: 'GitHubCopilotProvider' object has no attribute '_use_streaming', confirming the bug. All 4 pass after the fix.
  • 8 new tests in tests/test_unknown_config_keys.py cover: the
    allowlist is exactly the 13 keys, all-known-keys stays quiet, empty config
    stays quiet, a typo gets a did-you-mean suggestion, an unrelated key gets
    a bare mention (no suggestion), debug gets the targeted message (never
    did-you-mean), multiple unknown keys combine into one warning, and the
    sweep never raises on odd input shapes.
  • Full suite: baseline 1542 passed, 12 deselected (live) on unmodified
    05f640f -> 1554 passed, 12 deselected after this change -- exactly
    +12 (the tests added here), zero regressions.
  • ruff format --check, ruff check, and pyright (via this repo's own
    .venv/bin/pyright, matching make pyright) are all clean on every
    touched file.

Process note (external maintainer repo)

This PR is self-authored / landed at user direction because this module is
maintained by Mowri Mohan (@HDMowri) and I don't want a routine, low-risk fix sitting idle
if CI is green and the change is this narrow in scope. Mowri Mohan (@HDMowri) -- please
still give this a look whenever convenient
; happy to adjust or revert
anything that doesn't match your intent for this module. Nothing here should
need --admin to merge (branch protection / required-review status
permitting) -- only using elevated merge if review is gated and blocking an
otherwise-green, narrow fix; flagging that explicitly rather than doing it
silently.

🤖 Generated with Amplifier

… tighten ConfigField prompts

Three conservative, contract-respecting fixes surfaced by a deep-read of the
provider's config-handling code. All three follow existing patterns already
established in this module; none touch session_config, the singleton, or
add new ConfigFields.

1. use_streaming string coercion (provider.py)
   `self.config.get("use_streaming", True)` was read raw at call time, so a
   config-provided string "false" was truthy (`bool("false") is True`) and
   silently kept streaming enabled. Parsed once in `__init__` as
   `self._use_streaming`, reusing the module's own `_parse_raw_flag` helper
   verbatim -- the exact same pattern already used for `self._raw` and
   `self._enable_long_context` two lines above. `session_config["streaming"]`
   stays hard-pinned `True` in sdk_adapter/client.py (deny-destroy contract);
   this flag only gates Amplifier-side llm:stream_* event emission and is
   untouched.

2. Warn-only unknown-config-key sweep at mount() (__init__.py)
   A typo'd or stale config key (e.g. `us_streaming`) was silently inert with
   no signal to the operator. Added `_warn_unknown_config_keys`, called from
   `mount()` (guarded, never raises, never blocks mount). The allowlist is
   exactly the 13 keys this provider (or a live collaborator) recognizes: the
   11 keys read directly via `self.config.get(...)` / `config.get(...)` in
   provider.py, plus `priority` (read live by loop-streaming's own provider
   selection off this same config dict) and `extra_request_params`
   (app-cli-reserved) -- both deliberately NOT flagged as unknown since they
   are genuinely in use elsewhere. Unrecognized keys get a `difflib`
   did-you-mean suggestion when one exists. The literal key `debug` (present
   in ~9 of the maintainer's own test fixtures, genuinely unread anywhere in
   this provider) gets an honest targeted message ("not read by this
   provider") instead of a nonsensical did-you-mean guess.

3. ConfigField prompt-text tightening (provider.py get_info())
   Shortened the three existing ConfigField prompts (github_token,
   enable_long_context, reasoning_effort) for consistency/brevity. No ids,
   field_types, defaults, required/requires_model flags, choices, or field
   order changed -- get_info:MUST:6 (exact reasoning_effort choices list,
   position immediately after enable_long_context) is preserved verbatim.
   Updated the one test assertion in test_config_field_conformance.py that
   pinned the old github_token prompt text to match.

Deliberately NOT done (out of the safe list, per contract):
- No new ConfigFields added (provider-protocol.md:112-128 is a closed list;
  the app-cli model picker already collects the model).
- No `extra_request_params` ConfigField (deny-destroy.md:157 forbids
  session-config knobs becoming a YAML setting).
- No mount-time model/effort heuristics, no session_config changes, no
  singleton changes, no packaged YAML default changes.

Testing:
- Fail-before proof: a standalone repro plus 4 new regression tests
  (test_provider_streaming_contract.py::TestUseStreamingConfig) were run
  against unmodified code first and failed with
  `AttributeError: 'GitHubCopilotProvider' object has no attribute
  '_use_streaming'`, confirming the bug. All 4 pass after the fix.
- 8 new tests (test_unknown_config_keys.py) cover the allowlist shape, the
  quiet-on-known-keys case, did-you-mean suggestions, the `debug` special
  case, and multi-key combination.
- Full suite: baseline 1542 passed / 12 deselected (live) ->
  1554 passed / 12 deselected after (exactly +12 = the tests added here;
  zero regressions).
- `ruff format`, `ruff check`, and `pyright` (via the repo's own
  `.venv/bin/pyright`, matching `make pyright`) are clean on every touched
  file.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Contributor Author

Process note (external maintainer repo, admin-at-user-direction):

This module is maintained by Mowri Mohan (@HDMowri). This PR is self-authored and being
landed at the direction of the person operating this session (not
autonomously) -- CI is green (pytest (py3.11), pytest (py3.12),
license/cla all passing) and the change is a narrow, conservative fix
(see PR description for the full deep-read rationale and what was
deliberately left out of scope).

Branch protection on this repo requires review before merge
(reviewDecision: REVIEW_REQUIRED). I have admin access to this repo and
will use --admin only to satisfy that review-gate on an otherwise-green,
narrow, already-tested change -- not to bypass any failing or pending
check.

Mowri Mohan (@HDMowri) -- please review whenever convenient, even though this will
already be merged by the time you see this.
Happy to follow up with a
revert or amendment PR for anything that doesn't match your intent for this
module. The branch (fix/use-streaming-coercion-unknown-config-sweep) is
kept (not deleted) after merge in case you want to inspect it directly.

@bkrabach
Brian Krabach (bkrabach) merged commit ce42a83 into main Aug 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants