feat(config): round-trip user-owned extra_request_params through provider edit - #286
Merged
Brian Krabach (bkrabach) merged 1 commit intoAug 29, 2026
Merged
Conversation
…ider edit
Provider modules are adopting an extra_request_params config key -- an
owner-beware dict merged verbatim into every API request, for parameters
the module itself doesn't wrap. Users maintain this key by hand in
settings.yaml.
Problem: every provider config-rewrite path rebuilds a provider's config
purely from configure_provider()'s ConfigField schema answers, then
assigns the rebuilt dict wholesale over the old one. Any key not
declared as a ConfigField -- including extra_request_params -- was
silently dropped on reconfigure, defeating the whole point of a
hand-maintained passthrough bag.
Fix: add _preserve_reserved_keys(old, new) in provider_config_utils.py,
a single well-named helper with RESERVED_PROVIDER_CONFIG_KEYS =
("extra_request_params",) as its only (deliberately narrow) entry.
Apply it at every seam that replaces an EXISTING provider instance's
config in commands/provider.py:
* provider_edit() -- primary CLI edit path
* _manage_edit_provider() -- interactive manage-loop edit path
* provider_add() -- same-module replace-without-id branch
(covers a concurrent-write race that could otherwise drop the key)
* _manage_add_provider() -- interactive manage-loop, same branch
extra_request_params is never a ConfigField: never prompted for, never
displayed in the wizard, never validated. A fresh add (no prior config)
never invents the key. Any other non-schema key continues to be
dropped on edit exactly as before -- the preservation is deliberately
narrow.
Tests (tests/test_provider_config_reserved_keys.py):
(a) edit flow preserves extra_request_params verbatim (CLI + manage-loop)
(b) edit flow still drops an unrelated non-schema key
(c) the wizard never prompts for extra_request_params, even with an
empty config_fields schema
(d) a fresh add never invents the key (CLI + manage-loop)
(e) direct unit coverage of _preserve_reserved_keys()
Verified fail-before/pass-after via git stash: with the implementation
stashed, 7 of 11 new tests fail (the seams drop the key; the helper is
undefined); restored, all 11 pass. Full suite: 1508 passed (1497
baseline + 11 new), 1 skipped, 13 deselected, 1 xfailed -- no
regressions.
Also documents the extra_request_params convention in README.md.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Collaborator
Author
|
All required checks are green (license/cla + full 9-job pytest/integration matrix — see checks tab). This PR is self-authored and merged via the maintainer admin-merge path, executed by automation acting at the repo maintainer's explicit direction, per the documented process for this class of change. Merging with `--admin` to bypass the required-review gate (no other reviewers available in this workflow) since all status checks passed and no check is being overridden. |
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.
Summary
Provider modules are adopting an
extra_request_paramsconfig key -- an owner-beware dict merged verbatim into every API request, for parameters the module itself doesn't wrap. Users maintain this key by hand insettings.yaml.Problem: every provider config-rewrite path (
configure_provider()and its callers inprovider_config_utils.py/commands/provider.py) rebuilds a provider's config purely from the module's declaredConfigFieldschema answers, then assigns the rebuilt dict wholesale over the old one. Any key not declared as aConfigField-- includingextra_request_params-- was silently dropped on reconfigure, defeating the whole point of a hand-maintained passthrough bag.Fix: a single, narrow helper --
_preserve_reserved_keys(old, new)inamplifier_app_cli/provider_config_utils.py, backed byRESERVED_PROVIDER_CONFIG_KEYS = ("extra_request_params",)-- carries the key forward verbatim from the prior config into the rebuilt one, if present. Applied at every seam inamplifier_app_cli/commands/provider.pythat replaces an existing provider instance's config:provider_edit()(commands/provider.py:946pre-patch) -- primaryprovider editCLI path_manage_edit_provider()(commands/provider.py:1502pre-patch) -- interactiveprovider manageedit pathprovider_add()same-module replace-without-id branch -- covers a concurrent-write race that could otherwise drop the key_manage_add_provider()same branch, interactive-loop variantextra_request_paramsis never aConfigField: never prompted for, never displayed in the wizard, never validated. A fresh add (no prior config) never invents the key. Any other non-schema key continues to be dropped on edit exactly as before -- the preservation is deliberately narrow (see test (b)).Tests
New file:
tests/test_provider_config_reserved_keys.pyextra_request_paramsverbatim (CLIprovider edit+ interactive_manage_edit_provider)some_stale_key) -- preservation stays narrowextra_request_params, even with an emptyconfig_fieldsschemaprovider add(no prior config) never invents the key (CLI + interactive)_preserve_reserved_keys()Fail-before/pass-after (via
git stash): with the implementation (provider_config_utils.py+commands/provider.pychanges) stashed, 7 of 11 new tests fail --the seams drop the key and the helper is undefined. Restored, all 11 pass.Full suite: 1508 passed (1497 baseline + 11 new), 1 skipped, 13 deselected, 1 xfailed -- no regressions.
Docs
Added a short paragraph to
README.md(Provider management section) documenting the convention:extra_request_paramsis user-owned, round-tripped by config tooling, never wizard-prompted, contents are the user's responsibility.Merge note
This PR is self-authored/merged by the repo maintainer (via automation acting at their direction). It will only be merged with
--adminafter all required checks are green -- never over a failing check.🤖 Generated with Amplifier