fix: expand configured provider values during discovery

AGENTS.md

@@ -162,6 +162,15 @@ Seed byte-preservation installer tests with explicit LF and CRLF bytes, not
 text-mode writes that translate newlines. Assert the original prefix survives
 the first install and the entire file is identical after a repeat install.
 
+## Lightweight provider configuration
+
+Model discovery and login must expand an independent full configuration copy
+once with `lib.env_vars.expand_env_vars`, before deriving constructor arguments.
+Do not discard saved account settings or expand returned environment values a
+second time. Keep the helper lightweight; importing `runtime.config` from the
+provider loader creates a cycle. Run `tests/test_provider_loader_configuration.py`
+and `tests/test_env_vars.py` for this boundary.
+
 ## Interactive control-flow exits
 
 REPL exit commands return an action from `CommandProcessor`; only the normal

amplifier_app_cli/lib/env_vars.py

@@ -0,0 +1,28 @@
+"""Environment expansion shared by session setup and lightweight providers."""
+
+import os
+import re
+from typing import Any
+
+
+ENV_PATTERN = re.compile(r"\$\{([^}:]+)(?::([^}]*))?}")
+
+
+def expand_env_vars(config: dict[str, Any]) -> dict[str, Any]:
+    """Expand ${VAR} references within configuration values."""
+
+    def replace_value(value: Any) -> Any:
+        if isinstance(value, str):
+            return ENV_PATTERN.sub(_replace_match, value)
+        if isinstance(value, dict):
+            return {k: replace_value(v) for k, v in value.items()}
+        if isinstance(value, list):
+            return [replace_value(item) for item in value]
+        return value
+
+    def _replace_match(match: re.Match[str]) -> str:
+        var_name = match.group(1)
+        default = match.group(2)
+        return os.environ.get(var_name, default if default is not None else "")
+
+    return replace_value(config)

amplifier_app_cli/provider_loader.py

@@ -15,6 +15,7 @@
 from pathlib import Path
 from typing import TYPE_CHECKING, Any
 
+from .lib.env_vars import expand_env_vars
 from .provider_diagnostics import invoke_list_models
 
 if TYPE_CHECKING:
@@ -310,19 +311,21 @@ def _try_instantiate_provider(
         Provider instance, or None when its signature cannot accept the config.
         Provider constructor validation/runtime errors propagate unchanged.
     """
-    collected_config = collected_config or {}
+    # Match session expansion before either constructor handoff. Expand once:
+    # values returned by the environment may themselves contain literal ${...}.
+    # Deepcopy also isolates mutable values the expander does not traverse.
+    collected_config = expand_env_vars(deepcopy(collected_config or {}))
 
-    # Extract connection values from collected config
-    # Resolve ${VAR} placeholders to actual environment values
+    # Derive standalone arguments from the same expanded configuration.
     raw_base_url = collected_config.get("base_url") or collected_config.get(
         "azure_endpoint"
     )
     raw_host = collected_config.get("host")
     raw_api_key = collected_config.get("api_key")
 
-    base_url = _resolve_env_placeholder(raw_base_url) or "http://placeholder"
-    host = _resolve_env_placeholder(raw_host) or "http://localhost:11434"
-    api_key = _resolve_env_placeholder(raw_api_key) or ""
+    base_url = raw_base_url or "http://placeholder"
+    host = raw_host or "http://localhost:11434"
+    api_key = raw_api_key or ""
 
     # Bind before construction: a TypeError *inside* a valid constructor is
     # a provider failure, not permission to retry with different/default
@@ -344,7 +347,7 @@ def _try_instantiate_provider(
         # Some constructors normalize nested config in place. Discovery and
         # login must not mutate the caller's saved configuration or the
         # values the wizard later persists.
-        kwargs["config"] = deepcopy(collected_config)
+        kwargs["config"] = collected_config
     elif collected_config:
         # Metadata-only, no-argument facades remain usable for get_info(),
         # but cannot act as a configured provider for login/model discovery.

amplifier_app_cli/runtime/config.py

@@ -6,7 +6,6 @@
 import copy
 import logging
 import os
-import re
 from pathlib import Path
 from typing import TYPE_CHECKING
 from typing import Any
@@ -15,6 +14,7 @@
 from rich.console import Console
 
 from ..lib.bundle_loader.discovery import WELL_KNOWN_BUNDLES
+from ..lib.env_vars import ENV_PATTERN, expand_env_vars
 from ..lib.settings import AppSettings, NotificationFlags, get_custom_routing_dir
 from ..lib.merge_utils import merge_module_items
 from ..lib.merge_utils import merge_tool_configs
@@ -1216,9 +1216,6 @@ def _merge_module_lists(
     return result
 
 
-ENV_PATTERN = re.compile(r"\$\{([^}:]+)(?::([^}]*))?}")
-
-
 async def _validate_provider_credentials(
     providers: list[Any],
     *,
@@ -1228,7 +1225,7 @@ async def _validate_provider_credentials(
     """Fail loudly, before session mount, when a provider instance's
     configured credential placeholder resolves to nothing.
 
-    Why this exists: ``expand_env_vars`` (below) treats an unset ``${VAR}``
+    Why this exists: ``expand_env_vars`` treats an unset ``${VAR}``
     as an empty string. Several provider modules treat an empty/absent
     ``api_key`` config value as "not configured" and fall back to their own
     canonical ambient env var (e.g. ``OPENAI_API_KEY``). For a *separate*
@@ -1329,26 +1326,6 @@ async def _validate_provider_credentials(
             )
 
 
-def expand_env_vars(config: dict[str, Any]) -> dict[str, Any]:
-    """Expand ${VAR} references within configuration values."""
-
-    def replace_value(value: Any) -> Any:
-        if isinstance(value, str):
-            return ENV_PATTERN.sub(_replace_match, value)
-        if isinstance(value, dict):
-            return {k: replace_value(v) for k, v in value.items()}
-        if isinstance(value, list):
-            return [replace_value(item) for item in value]
-        return value
-
-    def _replace_match(match: re.Match[str]) -> str:
-        var_name = match.group(1)
-        default = match.group(2)
-        return os.environ.get(var_name, default if default is not None else "")
-
-    return replace_value(config)
-
-
 def inject_user_providers(config: dict, prepared_bundle: "PreparedBundle") -> None:
     """Inject user-configured providers into bundle's mount plan.
 

tests/test_env_vars.py

@@ -0,0 +1,104 @@
+"""Pin the session expansion contract reused by lightweight provider loading."""
+
+from copy import deepcopy
+
+import pytest
+
+from amplifier_app_cli.lib.env_vars import ENV_PATTERN, expand_env_vars
+
+
+def test_runtime_retains_the_shared_expansion_exports():
+    from amplifier_app_cli.runtime import config
+
+    assert config.expand_env_vars is expand_env_vars
+    assert config.ENV_PATTERN is ENV_PATTERN
+
+
+@pytest.mark.parametrize(
+    ("env_value", "value", "expected"),
+    [
+        (None, "${FIXTURE_VALUE}", ""),
+        (None, "${FIXTURE_VALUE:default}", "default"),
+        (None, "${FIXTURE_VALUE:}", ""),
+        ("present", "${FIXTURE_VALUE:default}", "present"),
+        ("", "${FIXTURE_VALUE}", ""),
+        ("", "${FIXTURE_VALUE:default}", ""),
+        # ':' supplies a default; shell-style ':-' includes the '-' literally.
+        (None, "${FIXTURE_VALUE:-default}", "-default"),
+        ("", "${FIXTURE_VALUE:-default}", ""),
+        (
+            None,
+            "${FIXTURE_VALUE:https://example.invalid:8443/v1}",
+            "https://example.invalid:8443/v1",
+        ),
+        ("present", "$FIXTURE_VALUE", "$FIXTURE_VALUE"),
+        ("present", "${FIXTURE_VALUE", "${FIXTURE_VALUE"),
+        ("present", "${}", "${}"),
+        ("present", "$${FIXTURE_VALUE}", "$present"),
+    ],
+)
+def test_expand_env_vars_placeholder_defaults_and_empty_values(
+    monkeypatch, env_value, value, expected
+):
+    monkeypatch.delenv("FIXTURE_VALUE", raising=False)
+    if env_value is not None:
+        monkeypatch.setenv("FIXTURE_VALUE", env_value)
+    config = {"value": value}
+    original = deepcopy(config)
+    assert expand_env_vars(config) == {"value": expected}
+    assert config == original
+
+
+def test_expand_env_vars_walks_dict_and_list_values_not_keys_or_tuples(monkeypatch):
+    monkeypatch.setenv("FIXTURE_VALUE", "expanded")
+    monkeypatch.setenv("FIXTURE_HOST", "example.invalid")
+    monkeypatch.delenv("FIXTURE_UNKNOWN", raising=False)
+    config = {
+        "${FIXTURE_VALUE}": {
+            "values": [
+                "${FIXTURE_VALUE}",
+                {"endpoint": "https://${FIXTURE_HOST}/${FIXTURE_VALUE}/v1"},
+                ["${FIXTURE_UNKNOWN}", "${FIXTURE_VALUE}"],
+            ]
+        },
+        "tuple": ("${FIXTURE_VALUE}", ["${FIXTURE_VALUE}"]),
+        "bool": False,
+        "int": 7,
+        "float": 1.5,
+        "none": None,
+    }
+    original = deepcopy(config)
+    expanded = expand_env_vars(config)
+    assert expanded == {
+        "${FIXTURE_VALUE}": {
+            "values": [
+                "expanded",
+                {"endpoint": "https://example.invalid/expanded/v1"},
+                ["", "expanded"],
+            ]
+        },
+        "tuple": ("${FIXTURE_VALUE}", ["${FIXTURE_VALUE}"]),
+        "bool": False,
+        "int": 7,
+        "float": 1.5,
+        "none": None,
+    }
+    assert expanded["tuple"] is config["tuple"]
+    for key in ("bool", "int", "float", "none"):
+        assert type(expanded[key]) is type(config[key])
+    assert config == original
+
+
+def test_expand_env_vars_substitutes_each_original_placeholder_only_once(monkeypatch):
+    monkeypatch.setenv("FIXTURE_FIRST", "${FIXTURE_SECOND}")
+    monkeypatch.setenv("FIXTURE_SECOND", "second-value")
+    config = {
+        "value": "prefix-${FIXTURE_FIRST}-${FIXTURE_SECOND}",
+        "nested": ["${FIXTURE_FIRST}"],
+    }
+    original = deepcopy(config)
+    assert expand_env_vars(config) == {
+        "value": "prefix-${FIXTURE_SECOND}-second-value",
+        "nested": ["${FIXTURE_SECOND}"],
+    }
+    assert config == original

tests/test_provider_instance_credentials.py

@@ -2355,20 +2355,24 @@ def test_no_kernel_level_env_var_rederivation():
     docs/designs/provider-instance-credentials.md §3.
 
     Scoped to the specific runtime-resolution functions §3 identified
-    (``expand_env_vars`` in runtime/config.py, ``_resolve_env_placeholder``
-    in provider_loader.py) rather than a whole-file grep: provider_loader.py
+    (the shared ``expand_env_vars`` helper and provider construction, plus the
+    legacy ``_resolve_env_placeholder`` compatibility helper) rather than a
+    whole-file grep: provider_loader.py
     legitimately *defines* and uses ``get_provider_info`` elsewhere in the
     file for wizard/prompt-time field derivation (§3: "type-level
     declarations consumed at authoring/prompt time only") -- a whole-file
     check would false-positive on the function's own name.
     """
     import inspect
 
-    from amplifier_app_cli.provider_loader import _resolve_env_placeholder
+    from amplifier_app_cli.provider_loader import (
+        _resolve_env_placeholder,
+        _try_instantiate_provider,
+    )
     from amplifier_app_cli.runtime.config import expand_env_vars
 
     forbidden = ("get_provider_info", "ConfigField", "credential_env_vars")
-    for fn in (expand_env_vars, _resolve_env_placeholder):
+    for fn in (expand_env_vars, _try_instantiate_provider, _resolve_env_placeholder):
         source = inspect.getsource(fn)
         for name in forbidden:
             assert name not in source, (