Skip to content

fix(provider): wizard honors default=None — never force-writes choice/boolean values (in_memory defect) - #284

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/wizard-unset-choice-fields
Aug 28, 2026
Merged

fix(provider): wizard honors default=None — never force-writes choice/boolean values (in_memory defect)#284
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/wizard-unset-choice-fields

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Root cause

_prompt_for_field's choice branch (in amplifier_app_cli/provider_config_utils.py) had no representation for "unset": it computed effective_value = existing_value or default and then hard-coded default_choice = "1". That meant a provider ConfigField declaring default=None (deliberately, to signal "leave unset — use provider/model default") with choices=[...] silently collapsed to choices[0] and always wrote a value on Enter.

Causal chain of the real-world defect

  1. provider-openai declares prompt_cache_retention with default=None + choices=["in_memory", "24h"] — the field's own source comment says None means "leave unset."
  2. The wizard's choice-field renderer force-selected choices[0] regardless.
  3. settings.yaml got prompt_cache_retention: in_memory written on every fresh configure, even though the user never chose it.
  4. gpt-5.6 models reject in_memory at runtime, producing a drop-warning every session.

The same bug force-writes text_verbosity=low for provider-openai, and would force-write any other provider's default=None choice field the moment it's declared (e.g. an anthropic reasoning_effort field shaped the same way).

A related latent bug lived in the boolean branch: default and default.lower() with default=None evaluates to None (short-circuit and), and Confirm.ask(default=None) still accepts that as a default — so pressing Enter wrote the literal string "none" into config.

Fixes (all in app-cli; no provider or core changes)

  1. Choice branch (provider_config_utils.py:610-649): when default is None and the field is not required, prepend a "(leave unset - use provider/model default)" option and make it the default selection. Selecting it returns None, so the caller omits the key — matching the existing text-field "may be absent" contract. A real existing_value still takes priority as the pre-selected option (offset to account for the prepended entry). required=True fields keep the original choices[0]-default behavior unchanged.

  2. Boolean branch (provider_config_utils.py:583-608): guard default is None explicitly instead of relying on and-chaining producing an accidental None. Confirm.ask is called with an explicit default=None; pressing Enter now returns None → key omitted. Never writes "none".

  3. Removed dead cli_overrides machinery in configure_provider(): the model=, endpoint=, deployment=, use_azure_cli= parameters and their handling (previously ~:716-719, :767-771, :846-851, :884, :891) had zero callers anywhere in the app or tests (grep-verified across amplifier_app_cli/ and tests/). use_default_credential/use_managed_identity targeted a field no provider even declares. non_interactive and every other parameter are untouched.

  4. Extracted the two byte-identical 42-line non-interactive-mode blocks (Phase 1 pre-model fields, Phase 3 post-model fields) into one _apply_non_interactive_field() helper — mechanical, behavior-preserving refactor.

Tests (tests/test_provider_config_unset_fields.py, 22 new)

  • Regression: choice field default=None + required=False omits the key on Enter (the in_memory defect) instead of force-writing choices[0].
  • Choice field with a real default: unchanged behavior (no unset option).
  • Choice field with existing_value: preserved as the default selection, positioned after the prepended unset option.
  • Boolean default=None: key omitted, never writes "none".
  • Full _should_show_field predicate matrix: literal / contains: / not_contains: / startswith: / not_startswith:, missing key, multiple conditions.
  • requires_model Phase-1/Phase-3 split: a requires_model field's show_when is proven to see default_model only after model selection (Phase 2) — spied via _should_show_field.
  • End-to-end exact-key-set assertion for a full configure_provider() run against a mocked provider schema (asserts the omitted fields are truly absent, not just falsy).
  • Dead-parameter removal: model=/endpoint=/deployment=/use_azure_cli= are no longer accepted parameters.

Test results

Full suite: 1491 passed, 1 skipped, 13 deselected, 1 xfailed (baseline 1469 passed + 22 new tests; zero regressions).

$ python3 -m pytest -q
1491 passed, 1 skipped, 13 deselected, 1 xfailed, 3 warnings in 7.02s

🤖 Generated with Amplifier

…/boolean values

Root cause: `_prompt_for_field`'s choice branch had no representation for
"unset". It computed `effective_value = existing_value or default` and then
hard-coded `default_choice = "1"`, so a provider `ConfigField` declaring
`default=None` (deliberately, to mean "leave unset — use provider/model
default") with `choices=[...]` silently collapsed to `choices[0]` and ALWAYS
wrote a value on Enter.

Causal chain of the real-world defect: provider-openai declares
`prompt_cache_retention` with `default=None` + `choices=["in_memory","24h"]`
-> the wizard force-wrote `in_memory` into settings.yaml -> gpt-5.6 models
reject that value at runtime, producing a drop-warning every session. The
same bug force-wrote `text_verbosity=low` for openai and would force-write
similarly-shaped anthropic choice fields the moment they declared
`default=None`. The boolean branch had a related latent bug:
`default and default.lower()` with `default=None` evaluates to `None`
(short-circuit), which `Confirm.ask(default=None)` still accepts as a
default — so pressing Enter wrote the literal string `"none"`.

Fixes (amplifier_app_cli/provider_config_utils.py):

1. Choice branch (_prompt_for_field): when `default is None` and the field
   is not required, prepend a "(leave unset — use provider/model default)"
   option and make it the default selection; choosing it returns None so the
   caller omits the key — matching the existing text-field "may be absent"
   contract. A real existing_value still takes priority as the pre-selected
   option. required=True fields keep the original choices[0]-default
   behavior unchanged.

2. Boolean branch (_prompt_for_field): guard `default is None` explicitly.
   Confirm.ask is now called with an explicit default=None (not a computed
   None from `and`-chaining) and pressing Enter returns None -> key omitted.
   Never writes "none".

3. Removed the dead `cli_overrides` machinery in `configure_provider`: the
   `model=`, `endpoint=`, `deployment=`, `use_azure_cli=` parameters and
   their handling had zero callers anywhere in the app or tests
   (survey-verified by grep across amplifier_app_cli/ and tests/;
   `use_default_credential`/`use_managed_identity` target fields no provider
   even declares). `non_interactive` and all other parameters are untouched.

4. Extracted the two byte-identical 42-line non-interactive-mode blocks
   (Phase 1 pre-model fields, Phase 3 post-model fields) into a single
   `_apply_non_interactive_field` helper — mechanical, behavior-preserving.

Tests (tests/test_provider_config_unset_fields.py, 22 new):
  - Regression: choice field default=None + not required omits the key on
    Enter (the in_memory defect) instead of force-writing choices[0].
  - Choice field with a real default: unchanged behavior (no unset option).
  - Choice field with existing_value: preserved as the default selection,
    positioned after the prepended unset option.
  - Boolean default=None: key omitted, never writes "none".
  - Full `_should_show_field` predicate matrix: literal/contains/
    not_contains/startswith/not_startswith, missing key, multiple conditions.
  - requires_model Phase-1/Phase-3 split: a requires_model field's show_when
    is proven to see `default_model` only after model selection (Phase 2).
  - End-to-end exact-key-set assertion for a full configure_provider() run
    against a mocked provider schema.
  - Dead-parameter removal: model=/endpoint=/deployment=/use_azure_cli= are
    no longer accepted parameters.

Full suite: 1491 passed, 1 skipped, 13 deselected, 1 xfailed (baseline 1469
passed + 22 new tests; zero regressions).

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

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

Copy link
Copy Markdown
Collaborator Author

Maintainer admin-merge: self-authored, self-approval impossible

Basis: Root cause of the reported gpt-5.6-luna prompt_cache_retention: in_memory defect. The wizard's choice renderer had no "leave unset" concept and collapsed None-defaults to choices[0]; it now renders (leave unset) as the default and omits the key entirely when left unset. Added a matching boolean None guard, and removed the dead cli_overrides machinery.

Testing: 22 new tests added covering previously-zero-coverage code paths (choice branch, show_when matrix, requires_model phase split, exact-key-set assertions). Full suite: 1491 passed.

DTU validation: Control run reproduced the defect exactly (force-wrote in_memory + text_verbosity=low; runtime emitted a "Dropping unsupported config" warning). Treatment run omitted both keys with zero warnings. Leave-unset behavior was separately verified on Anthropic's reasoning_effort field.

CI: All 9 matrix jobs + CLA green (see checks tab).

This PR is self-authored. Self-approval is not possible under branch protection, so this merge is being performed via gh pr merge --admin at explicit user direction, per the established documented maintainer admin-merge pattern (~30 PRs merged this way recently). Recorded here for auditability.

@bkrabach
Brian Krabach (bkrabach) merged commit 1617218 into main Aug 28, 2026
9 checks passed
Brian Krabach (bkrabach) pushed a commit that referenced this pull request Aug 28, 2026
Inline /amplifier-config skill (root provider selection vs spawned-session routing vs bundle composition vs settings-scope precedence): inspects effective config with provenance, proposes smallest safe change with blast-radius/rollback, applies only authorized edits.

Verified against this week's changes (#284 wizard None-default rendering, provider-openai#69 model-gated ConfigFields, #281 multi-instance credential flow) -- none hardcoded in SKILL.md; it correctly defers version-sensitive facts to live inspection + app-cli:cli-expert. Wiring confirmed via auto-discovery in _ensure_default_skills_dirs() (same mechanism as goalify/goal-batch/ten-lane-highway) and new tests (test_packaged_amplifier_config_discovery_and_invocation, test_amplifier_config_skill_contract). Full suite: 1493 passed (main's 1491 + 2 new tests), zero regressions.

Admin-merge at team direction: fork PR (no push access to author's branch to rebase), ruleset requires 1 approving review with none yet recorded, CI 9/9 green, content review complete -- see PR comment for full due-diligence writeup.

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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