From dbcf719c330dc6546020a428fa4315ea8bc3dd53 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Tue, 15 Sep 2026 07:01:17 -0700 Subject: [PATCH] fix: config overrides reach session.context and session.orchestrator resolve_bundle_config() walked providers/tools/hooks at the bundle root and inside each agent, and nothing else. The context manager and orchestrator are declared at session.context / session.orchestrator as single module entries rather than lists, so the walk never visited them. The consequence was total, not partial: NO context-manager or orchestrator setting could be overridden from settings.yaml. Writing overrides: context-simple: config: max_tokens: 500000 did nothing, silently -- no error, no warning, no effect. That contradicted the rule the loop is built on, stated in _apply_config_overrides_to_section's own docstring: "overrides..config is keyed by module identity, not by mount location, so it must reach a module wherever it's declared." Holding a dict instead of a list is a mount-location accident, which is exactly what that rule exists to rule out. Adds _apply_config_overrides_to_entry(), the single-entry sibling, which delegates to the list version so both paths share one definition of what an override means -- normalize, match by module id, deep-merge with the override winning, preserve other keys, return the original object untouched on no match. This is what makes context-simple's new max_tokens / max_tokens_fallback settable by a user without editing a bundle. Tests: 9 new, exercising the real seam rather than a copy of its logic, including agreement between the entry and section paths. 2335 passed. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/runtime/config.py | 34 ++++ tests/test_session_module_config_overrides.py | 149 ++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 tests/test_session_module_config_overrides.py diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 0cf4974c..79a9e182 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -222,6 +222,23 @@ def _on_progress(action: str, detail: str) -> None: agent_section, config_overrides ) + # `session.context` and `session.orchestrator` are single module + # entries, not lists, which is the ONLY reason they were missed by the + # list walk above -- a mount-location accident, exactly what the + # "keyed by module identity, not mount location" rule exists to rule + # out. Until this, no context-manager or orchestrator setting could be + # overridden from settings.yaml at all: `overrides.context-simple.config` + # was silently ignored. + session_section = bundle_config.get("session") + if isinstance(session_section, dict): + for session_key in ("context", "orchestrator"): + entry = session_section.get(session_key) + if not entry: + continue + session_section[session_key] = _apply_config_overrides_to_entry( + entry, config_overrides + ) + # Apply provider overrides provider_overrides = app_settings.get_provider_overrides() if provider_overrides: @@ -524,6 +541,23 @@ def _map_id_to_instance_id( return result +def _apply_config_overrides_to_entry( + entry: Any, config_overrides: dict[str, Any] +) -> Any: + """Apply `overrides..config` to ONE module entry. + + The single-entry sibling of :func:`_apply_config_overrides_to_section`, for + mount points that hold one module rather than a list -- `session.context` + and `session.orchestrator`. + + Delegates to the list version so both paths share one definition of what an + override means (normalize, match by module id, deep-merge with the override + winning, preserve every other key, return the original object untouched + when nothing matches). + """ + return _apply_config_overrides_to_section([entry], config_overrides)[0] + + def _apply_config_overrides_to_section( section: list[Any], config_overrides: dict[str, Any] ) -> list[Any]: diff --git a/tests/test_session_module_config_overrides.py b/tests/test_session_module_config_overrides.py new file mode 100644 index 00000000..76a782cd --- /dev/null +++ b/tests/test_session_module_config_overrides.py @@ -0,0 +1,149 @@ +"""`overrides..config` must reach `session.context` / `session.orchestrator`. + +Why this file exists +-------------------- +`resolve_bundle_config()` walked `providers`, `tools` and `hooks` -- at the +bundle root and inside each agent -- and nothing else. The context manager and +the orchestrator are declared at `session.context` and `session.orchestrator`, +single module entries rather than lists, so the walk never visited them. + +The consequence was total, not partial: **no** context-manager or orchestrator +setting could be overridden from `settings.yaml`. Writing + + overrides: + context-simple: + config: + max_tokens: 500000 + +did nothing at all, silently -- no error, no warning, no effect. + +That contradicted the rule the override loop is built on, stated in +`_apply_config_overrides_to_section`'s own docstring: "`overrides..config` +is keyed by module identity, not by mount location, so it must reach a module +wherever it's declared." Being a dict instead of a list is a mount-location +accident, which is precisely what that rule exists to rule out. + +These tests exercise the real `_apply_config_overrides_to_entry` seam rather +than a copy of its logic, so they fail if the implementation drifts. +""" + +from __future__ import annotations + +from amplifier_app_cli.runtime.config import ( + _apply_config_overrides_to_entry, + _apply_config_overrides_to_section, +) + + +def _context_entry(**config) -> dict: + entry = { + "module": "context-simple", + "source": "git+https://github.com/microsoft/amplifier-module-context-simple@main", + } + if config: + entry["config"] = config + return entry + + +# --------------------------------------------------------------------------- +# 1. The defect, directly. +# --------------------------------------------------------------------------- + + +def test_override_reaches_a_session_context_entry(): + """The case that was silently ignored before.""" + entry = _context_entry() + overrides = {"context-simple": {"max_tokens": 500_000}} + + result = _apply_config_overrides_to_entry(entry, overrides) + + assert result["config"]["max_tokens"] == 500_000 + + +def test_override_reaches_an_orchestrator_entry(): + entry = {"module": "loop-streaming", "config": {"extended_thinking": True}} + overrides = {"loop-streaming": {"budget_warn_ratio": 0.5}} + + result = _apply_config_overrides_to_entry(entry, overrides) + + assert result["config"]["budget_warn_ratio"] == 0.5 + assert result["config"]["extended_thinking"] is True + + +# --------------------------------------------------------------------------- +# 2. Merge semantics match the list path exactly. +# --------------------------------------------------------------------------- + + +def test_override_merges_with_existing_config_rather_than_replacing_it(): + entry = _context_entry(max_tokens_fallback=300_000, compact_threshold=0.8) + overrides = {"context-simple": {"max_tokens": 500_000}} + + result = _apply_config_overrides_to_entry(entry, overrides) + + assert result["config"] == { + "max_tokens_fallback": 300_000, + "compact_threshold": 0.8, + "max_tokens": 500_000, + } + + +def test_override_wins_on_a_key_conflict(): + entry = _context_entry(max_tokens=300_000) + overrides = {"context-simple": {"max_tokens": 500_000}} + + result = _apply_config_overrides_to_entry(entry, overrides) + + assert result["config"]["max_tokens"] == 500_000 + + +def test_non_config_keys_are_preserved(): + entry = _context_entry(max_tokens=1) + overrides = {"context-simple": {"max_tokens": 2}} + + result = _apply_config_overrides_to_entry(entry, overrides) + + assert result["module"] == "context-simple" + assert result["source"] == entry["source"] + + +def test_entry_and_section_paths_agree(): + """One definition of "apply an override", reached two ways.""" + entry = _context_entry(compact_threshold=0.8) + overrides = {"context-simple": {"max_tokens": 500_000}} + + via_entry = _apply_config_overrides_to_entry(entry, overrides) + via_section = _apply_config_overrides_to_section([entry], overrides)[0] + + assert via_entry == via_section + + +# --------------------------------------------------------------------------- +# 3. Non-matches stay untouched. +# --------------------------------------------------------------------------- + + +def test_unrelated_override_leaves_the_entry_identical(): + entry = _context_entry(max_tokens=300_000) + + result = _apply_config_overrides_to_entry( + entry, {"some-other-module": {"whatever": 1}} + ) + + assert result is entry + + +def test_empty_overrides_leave_the_entry_identical(): + entry = _context_entry(max_tokens=300_000) + + assert _apply_config_overrides_to_entry(entry, {}) is entry + + +def test_bare_string_entry_is_tolerated(): + """Shorthand `context: context-simple` must not crash the walk.""" + result = _apply_config_overrides_to_entry( + "context-simple", {"context-simple": {"max_tokens": 500_000}} + ) + + assert result["module"] == "context-simple" + assert result["config"]["max_tokens"] == 500_000