From 86519ad124e60fc686117cf4daf6b5b6d3dd10c5 Mon Sep 17 00:00:00 2001 From: amplifier-lane Date: Wed, 2 Sep 2026 17:01:28 -0700 Subject: [PATCH 1/3] fix(resume): narrow the sub-session credential refresh to secrets only resume_sub_session re-applies live settings.yaml provider overrides onto a resumed sub-session's PERSISTED mount plan, for one stated reason (see the block's own comment at session_spawner.py): on-disk metadata has its secrets redacted, so a resumed session would otherwise send `Bearer [REDACTED]`. But the merge it used was the full one. The drop site, quoted at file:line: session_spawner.py:1005-1010 (pre-fix numbering) _live_provider_overrides = _live_settings.get_provider_overrides() if _live_provider_overrides: _refreshed_providers = _apply_provider_overrides( merged_config["providers"], _live_provider_overrides ) -> runtime/config.py:564 merged = merge_module_items(provider, override_map[key]) -> lib/merge_utils.py:149-152 if key == "config" and key in merged: merged["config"] = deep_merge(merged["config"], value) -> lib/merge_utils.py:64 "Deep merge two dicts, with overlay winning conflicts." base = the child's persisted provider config (priority: 0, installed by model_role/provider_preferences at spawn); overlay = the settings override (priority: 14 for the promoted provider on the measured host). Overlay wins, so the promotion was destroyed and the resumed leg silently re-resolved to whatever sits at settings priority 0. priority is not a secret. This is collateral damage from the metadata redaction security fix. MEASURED (model_performance-rc0, 2,078 session files, 22 GB, 12 capture roots): - 66 of 778 delegate sessions contain a session:resume; 39 (59%) change model across the boundary; 37/39 cheap -> expensive - all 39 report basis="priority" on BOTH sides -- a wipe, not a fallback - 0 of 179 ROOT resumes affected: a root plan has no promotion to lose - 402 of 29,702 captured requests (1.35%); worst single run 25.0% THE FIX Narrow the resume-time provider override to the keys redact_secrets() actually redacted, reusing amplifier_core's own SENSITIVE_KEYS so the two directions can never drift apart. Identity keys (module, id) are carried through so the override still matches its target; every other top-level key is dropped, so a settings override cannot rewrite `source` at resume time either. Scope: RESUME only. Root/fresh config assembly (resolve_bundle_config) still merges overrides in full -- there settings ARE the intended source of truth and there is no persisted child promotion to protect. The hook refresh in the same block is deliberately NOT narrowed; the reasoning is recorded inline. This also closes rc0 section 4.6, which it could only record as INFERRED-NOT-CONFIRMED: merge_utils.py:152 merges every settings key, so reasoning_effort was structurally exposed to the same wipe. The capture had "high" on both sides and could not observe it; the new test uses differing values and settles it. Tests: tests/test_narrow_overrides_to_secrets.py (13, no API calls). --- amplifier_app_cli/runtime/config.py | 109 ++++++++++ amplifier_app_cli/session_spawner.py | 37 +++- tests/test_narrow_overrides_to_secrets.py | 243 ++++++++++++++++++++++ 3 files changed, 388 insertions(+), 1 deletion(-) create mode 100644 tests/test_narrow_overrides_to_secrets.py diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 85358841..839cfa27 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -10,6 +10,7 @@ from typing import TYPE_CHECKING from typing import Any +from amplifier_core.utils.truncate import SENSITIVE_KEYS from rich.console import Console from ..lib.settings import AppSettings, NotificationFlags, get_custom_routing_dir @@ -571,6 +572,114 @@ def _apply_provider_overrides( return result +def _prune_to_secret_keys(value: Any) -> Any | None: + """Return a copy of ``value`` keeping ONLY secret-bearing branches. + + Companion to ``redact_secrets()`` (amplifier_core.utils.truncate), which + is what *creates* the problem this solves: persisting a session redacts + every key in ``SENSITIVE_KEYS`` to the literal ``"[REDACTED]"``. A resume + therefore needs to restore exactly those keys from live settings -- and + nothing else. Using the SAME key set in both directions is deliberate: + if redaction ever learns a new secret key, the refresh learns it too, with + no second list to keep in sync. + + Pruning rules: + - dict: keep a key outright if its name is a sensitive key; otherwise + recurse and keep the key only if something secret survives beneath it. + An empty result is reported as ``None`` (nothing to restore). + - list: kept WHOLE if any element carries a secret anywhere, else + dropped. Lists are *replaced* (not merged) by ``deep_merge``, so a + partially-pruned list would silently truncate the merged result -- + all-or-nothing is the only safe choice. + - scalars: never secret on their own (only a *key* marks a secret). + + Returns: + Pruned structure, or None when it holds no secret at any depth. + """ + if isinstance(value, dict): + kept: dict[str, Any] = {} + for key, sub_value in value.items(): + if isinstance(key, str) and key.lower() in SENSITIVE_KEYS: + kept[key] = sub_value + continue + pruned = _prune_to_secret_keys(sub_value) + if pruned is not None: + kept[key] = pruned + return kept or None + if isinstance(value, list): + if any(_prune_to_secret_keys(item) is not None for item in value): + return value + return None + return None + + +def narrow_overrides_to_secrets( + overrides: list[dict[str, Any]], +) -> list[dict[str, Any]]: + """Reduce settings overrides to their secret-bearing config keys only. + + WHY THIS EXISTS (model_performance-rc0 / -n1i) + --------------------------------------------- + ``resume_sub_session`` re-applies live ``settings.yaml`` provider + overrides onto a resumed sub-session's PERSISTED mount plan, for one + stated reason: on-disk metadata has its secrets redacted, so a resumed + session would otherwise send ``Bearer [REDACTED]``. + + But the merge it used was the full one (``merge_module_items`` -> + ``deep_merge``, "overlay winning conflicts"), so EVERY settings key -- + not just the secrets -- was re-imposed on the child's own plan. The + load-bearing casualty is ``config.priority``: a sub-session spawned with + ``model_role``/``provider_preferences`` carries ``priority: 0`` on the + promoted provider, and the settings priority overwrote it. The resumed + leg then silently re-resolved to whatever sits at settings priority 0. + Measured across a 2,078-session archive: 39 of 66 delegate resumes + changed model across the boundary, 37 of them cheap -> expensive, every + one reporting ``basis: "priority"`` on both sides (i.e. not a fallback -- + a wipe). Root sessions were untouched (0 of 179), exactly as the + mechanism predicts: a root plan has no promotion to lose. + + ``priority`` is not a secret. Narrowing the override to the keys that + were actually redacted restores the credential *without* handing settings + a second, unintended vote on provider resolution. + + Identity keys (``module``, ``id``) are carried through so the override + still matches its target entry; every other top-level key is dropped, so + a settings override cannot rewrite ``source`` or any sibling field at + resume time either. + + NOTE ON SCOPE: this is for the RESUME refresh only. Root/fresh config + assembly (``resolve_bundle_config``) still merges overrides in full -- + there the settings ARE the intended source of truth, and there is no + persisted child promotion to protect. + + Args: + overrides: Settings override entries (``{module, id?, config}``). + + Returns: + A new list holding only entries that carry at least one secret, each + narrowed to its secret-bearing config keys. Entries without secrets + are dropped entirely (they have nothing to restore). + """ + narrowed: list[dict[str, Any]] = [] + for override in overrides or []: + if not isinstance(override, dict) or "module" not in override: + continue + config = override.get("config") + if not isinstance(config, dict): + continue + secret_config = _prune_to_secret_keys(config) + if not secret_config: + continue + entry: dict[str, Any] = { + "module": override["module"], + "config": secret_config, + } + if override.get("id"): + entry["id"] = override["id"] + narrowed.append(entry) + return narrowed + + def _apply_hook_overrides( hooks: list[dict[str, Any]], overrides: list[dict[str, Any]] ) -> list[dict[str, Any]]: diff --git a/amplifier_app_cli/session_spawner.py b/amplifier_app_cli/session_spawner.py index c7507fc8..6f8ee4a7 100644 --- a/amplifier_app_cli/session_spawner.py +++ b/amplifier_app_cli/session_spawner.py @@ -1207,12 +1207,31 @@ async def resume_sub_session( _map_id_to_instance_id, deep_merge, expand_env_vars, + narrow_overrides_to_secrets, ) _live_settings = AppSettings() if merged_config.get("providers"): - _live_provider_overrides = _live_settings.get_provider_overrides() + # SECRETS ONLY -- see narrow_overrides_to_secrets() for the full + # rationale (model_performance-rc0 / -n1i). + # + # The unnarrowed merge re-imposed EVERY settings key on the + # child's own persisted mount plan. `config.priority` is the + # load-bearing casualty: a sub-session spawned with a + # model_role/provider_preferences promotion carries priority: 0 + # on the promoted provider, and the settings priority overwrote + # it -- so the resumed leg silently re-resolved to the settings + # priority-0 provider (measured: 39/66 delegate resumes changed + # model, 37 of them cheap -> expensive, basis="priority" on both + # sides). `reasoning_effort` and every other per-candidate config + # key were structurally exposed to the same wipe. + # + # Only the keys that redact_secrets() actually redacted need + # restoring here, so only those are allowed through. + _live_provider_overrides = narrow_overrides_to_secrets( + _live_settings.get_provider_overrides() + ) if _live_provider_overrides: _refreshed_providers = _apply_provider_overrides( merged_config["providers"], _live_provider_overrides @@ -1235,6 +1254,22 @@ async def resume_sub_session( # This is the piece that was previously MISSING: only providers # were refreshed, so a resumed sub-session kept sending # `Bearer [REDACTED]` for any hook/destination api_key. + # + # DELIBERATE ASYMMETRY with the provider refresh above, which is + # narrowed to secrets. Hooks are NOT narrowed, for two reasons: + # 1. Nothing in a hook entry carries per-session RESOLUTION + # state. The provider wipe mattered because `config.priority` + # decides which model a leg runs on; a hook has no analogue. + # 2. get_notification_hook_overrides() legitimately APPENDS + # hooks that are absent from the persisted plan (see + # _apply_hook_overrides). Narrowing to secrets would append + # those hooks stripped of `enabled`/`topic`/etc, breaking + # notifications on resumed sub-sessions to fix a defect not + # observed here. + # The same over-reach IS structurally possible for a hook whose + # config an agent overlay customised (settings would re-impose its + # own value at resume). No instance has been measured; narrowing + # this path needs its own evidence, not a speculative change. _config_overrides = _live_settings.get_config_overrides() _refreshed_hooks = merged_config["hooks"] if _config_overrides: diff --git a/tests/test_narrow_overrides_to_secrets.py b/tests/test_narrow_overrides_to_secrets.py new file mode 100644 index 00000000..f9cc0a32 --- /dev/null +++ b/tests/test_narrow_overrides_to_secrets.py @@ -0,0 +1,243 @@ +"""Unit coverage for the resume credential refresh narrowing (fix A). + +``narrow_overrides_to_secrets()`` is the guard that stops the resume-time +credential refresh from re-imposing every ``settings.yaml`` key on a resumed +sub-session's own persisted mount plan. The load-bearing casualty of the +unnarrowed merge was ``config.priority``: it overwrote the ``priority: 0`` +that ``model_role``/``provider_preferences`` installed at spawn, so the +resumed leg silently re-resolved to the settings priority-0 provider +(model_performance-rc0: 39 of 66 delegate resumes changed model, 37 of them +cheap -> expensive; 0 of 179 root resumes affected). + +End-to-end coverage through the real ``resume_sub_session()`` path lives in +``test_resume_preserves_provider_promotion.py``. + +No API calls anywhere in this module. +""" + +from __future__ import annotations + +from amplifier_app_cli.runtime.config import _apply_provider_overrides +from amplifier_app_cli.runtime.config import narrow_overrides_to_secrets + + +# --------------------------------------------------------------------------- +# Fixtures modelled on the rc0 capture +# 20260901-rebaseline/runs/val-rb-oai-sol-xhigh-s1-01 +# .../0000000000000000-25443a97b60d4965_anchors-amp-dev-git-ops +# leg 1: luna priority 0 / sol priority 1 -> 13 x gpt-5.6-luna +# leg 2: luna priority 14 / sol priority 0 -> 25 x gpt-5.6-sol +# --------------------------------------------------------------------------- + +PROMOTED = "provider-luna" # the cheap tier the role promoted +SETTINGS_ZERO = "provider-sol" # what settings puts at priority 0 + + +def _persisted_child_providers() -> list[dict]: + """The child's mount plan as spawn built it, then redaction persisted it.""" + return [ + { + "module": PROMOTED, + "config": { + "api_key": "[REDACTED]", + "default_model": "gpt-5.6-luna", + "priority": 0, + "reasoning_effort": "medium", + # Present ONLY in the preference's own config -- never in + # settings.yaml. These survived the wipe in the capture and + # are what proved the child's config was merged, not rebuilt. + "enable_response_chaining": "auto", + "prompt_cache_retention": "in_memory", + }, + }, + { + "module": SETTINGS_ZERO, + "config": { + "api_key": "[REDACTED]", + "default_model": "gpt-5.6-sol", + "priority": 1, + }, + }, + ] + + +def _live_settings_overrides() -> list[dict]: + """Live settings.yaml: real keys, plus the priorities that did the damage. + + ``reasoning_effort`` differs from the child's persisted value on purpose: + rc0 recorded that drift as INFERRED-NOT-CONFIRMED because the capture had + the same effort on both sides. Differing values settle it here. + """ + return [ + { + "module": PROMOTED, + "config": { + "api_key": "sk-live-luna", + "priority": 14, + "reasoning_effort": "high", + }, + }, + { + "module": SETTINGS_ZERO, + "config": { + "api_key": "sk-live-sol", + "priority": 0, + }, + }, + ] + + +def _by_module(providers: list[dict], module: str) -> dict: + return next(p for p in providers if p["module"] == module) + + +class TestRefreshNarrowedToSecrets: + """The resume credential refresh must restore secrets and nothing else.""" + + def test_promotion_survives_the_credential_refresh(self): + """FAILS BEFORE FIX A. + + Before: settings ``priority: 14`` deep-merged over the child's + ``priority: 0`` and the promotion was gone. + """ + refreshed = _apply_provider_overrides( + _persisted_child_providers(), + narrow_overrides_to_secrets(_live_settings_overrides()), + ) + + promoted = _by_module(refreshed, PROMOTED) + assert promoted["config"]["priority"] == 0, ( + "The child's spawn-time promotion (priority 0) must survive the " + "resume credential refresh. Settings priority is not a secret and " + "must not be re-imposed on a persisted child mount plan." + ) + assert _by_module(refreshed, SETTINGS_ZERO)["config"]["priority"] == 1, ( + "The spawn-time demotion of the settings priority-0 provider must " + "survive too -- otherwise both providers tie at 0." + ) + + def test_credentials_are_still_refreshed(self): + """The reason the refresh exists at all must keep working.""" + refreshed = _apply_provider_overrides( + _persisted_child_providers(), + narrow_overrides_to_secrets(_live_settings_overrides()), + ) + + assert _by_module(refreshed, PROMOTED)["config"]["api_key"] == "sk-live-luna" + assert ( + _by_module(refreshed, SETTINGS_ZERO)["config"]["api_key"] == "sk-live-sol" + ) + + def test_per_candidate_config_keys_survive(self): + """rc0 section 4.6: ``reasoning_effort`` was exposed to the same wipe. + + The capture could not observe it (preference and settings both said + "high" for the same provider id), so rc0 recorded it + INFERRED-NOT-CONFIRMED. Differing values settle it: the child's own + effort must win on its own plan. + """ + refreshed = _apply_provider_overrides( + _persisted_child_providers(), + narrow_overrides_to_secrets(_live_settings_overrides()), + ) + + assert _by_module(refreshed, PROMOTED)["config"]["reasoning_effort"] == "medium" + + def test_preference_only_keys_are_untouched(self): + """Keys absent from settings survived even before the fix; still do.""" + refreshed = _apply_provider_overrides( + _persisted_child_providers(), + narrow_overrides_to_secrets(_live_settings_overrides()), + ) + + promoted_config = _by_module(refreshed, PROMOTED)["config"] + assert promoted_config["enable_response_chaining"] == "auto" + assert promoted_config["prompt_cache_retention"] == "in_memory" + assert promoted_config["default_model"] == "gpt-5.6-luna" + + +class TestNarrowOverridesToSecrets: + """Unit coverage for the narrowing helper itself.""" + + def test_non_secret_keys_are_dropped(self): + narrowed = narrow_overrides_to_secrets( + [{"module": "provider-x", "config": {"api_key": "k", "priority": 3}}] + ) + assert narrowed == [{"module": "provider-x", "config": {"api_key": "k"}}] + + def test_entries_without_secrets_are_dropped_entirely(self): + assert ( + narrow_overrides_to_secrets( + [{"module": "provider-x", "config": {"priority": 3}}] + ) + == [] + ) + + def test_identity_keys_are_preserved(self): + """``id`` must survive or the override stops matching its target.""" + narrowed = narrow_overrides_to_secrets( + [ + { + "module": "provider-anthropic", + "id": "anthropic-sonnet", + "config": {"api_key": "k", "priority": 9}, + } + ] + ) + assert narrowed[0]["id"] == "anthropic-sonnet" + assert narrowed[0]["config"] == {"api_key": "k"} + + def test_non_identity_top_level_keys_are_dropped(self): + """A settings override must not rewrite ``source`` at resume time.""" + narrowed = narrow_overrides_to_secrets( + [ + { + "module": "provider-x", + "source": "git+https://example.invalid/other", + "config": {"api_key": "k"}, + } + ] + ) + assert "source" not in narrowed[0] + + def test_nested_secrets_are_kept_with_their_path(self): + narrowed = narrow_overrides_to_secrets( + [ + { + "module": "hooks-x", + "config": { + "enabled": True, + "auth": {"token": "t", "retries": 3}, + }, + } + ] + ) + assert narrowed[0]["config"] == {"auth": {"token": "t", "retries": 3}} + assert "enabled" not in narrowed[0]["config"] + + def test_lists_holding_secrets_are_kept_whole(self): + """deep_merge REPLACES lists; a partially-pruned list would truncate.""" + destinations = [{"url": "https://x.invalid", "api_key": "k"}] + narrowed = narrow_overrides_to_secrets( + [{"module": "hooks-x", "config": {"destinations": destinations}}] + ) + assert narrowed[0]["config"]["destinations"] == destinations + + def test_lists_without_secrets_are_dropped(self): + assert ( + narrow_overrides_to_secrets( + [{"module": "hooks-x", "config": {"targets": [{"url": "u"}]}}] + ) + == [] + ) + + def test_malformed_entries_are_skipped_not_raised(self): + assert ( + narrow_overrides_to_secrets( + [None, "provider-x", {"config": {"api_key": "k"}}, {"module": "m"}] + ) + == [] + ) + + def test_empty_input_is_empty_output(self): + assert narrow_overrides_to_secrets([]) == [] From 167e830ba8e07a048df06ec9ecdde1aec731470d Mon Sep 17 00:00:00 2001 From: amplifier-lane Date: Wed, 2 Sep 2026 17:02:56 -0700 Subject: [PATCH 2/3] fix(resume): thread model_role/provider_preferences through the delegate resume path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The narrowing in the previous commit stops the promotion being DESTROYED. This commit lets a resumed leg REBUILD it -- so it re-resolves against the current provider set, and says so honestly when it cannot. THE GAP, IN ONE GREP apply_provider_preferences_with_resolution appeared EXACTLY ONCE in session_spawner.py -- inside spawn_sub_session. It was not reachable from the resume path at all. The narrowing continued the whole way down (pre-fix numbering, rc0 section 2.2): hop spawn resume tool-delegate call site __init__.py:1509 prefs+role __init__.py:1444 neither capability invocation __init__.py:1771 spawn_fn __init__.py:2084 resume_fn app-cli capability :715,730 (…prefs…) :736,1253 -- 2 args app-cli implementation :231,241 spawn_sub_session :923 resume_sub_session promotion applied? :417-421 never WHAT THIS ADDS - resume_sub_session(..., provider_preferences=None, model_role=None) - both resume capability closures in session_spawner.py and the one in session_runner.py gain the same two optional keyword arguments, matching their child_spawn_capability siblings. Optional, so a caller still invoking (sub_session_id, instruction) is unaffected. - the resume path now applies apply_provider_preferences_with_resolution exactly as spawn_sub_session does. RECOVERY SOURCES -- why this reaches sessions nobody re-plumbed Preference precedence is: threaded by the caller > the persisted agent overlay > the persisted mount plan's own copy. The last two matter because the rc0 capture showed both were still sitting in the resumed session's config, "simply never consulted again". Recovering them means an existing caller that has not yet been taught to thread preferences keeps its promotion anyway; the threaded argument is what a caller gains when it is. FALLBACK IS NAMED, NEVER SILENT Per the rc0 acceptance criteria: when no preferred provider is mounted, the resume emits provider:fallback carrying reason, the requested preferences, where they came from, and the provider/model the leg actually landed on -- instead of silently re-resolving by settings priority. Promotion success is verified from the OUTCOME (a preferred provider at priority 0), not from the apply call's return value, so it stays honest across foundation versions. model_role is written onto the resumed config so the leg's routing hook resolves the same role the spawn leg was given. NOT IN THIS REPO: the matching change in microsoft/amplifier-foundation (tool-delegate's _resume_existing_session at __init__.py:1997 and its call site at :1444) is what makes the caller pass these through. The signatures here are additive and backward-compatible precisely so the two can land independently, and the recovery sources above mean the defect is fixed either way. Tests: tests/test_resume_preserves_provider_promotion.py (8, no API calls). Fail-before on the pre-fix tree: 7 failed / 1 passed, the headline assertion reproducing the capture exactly -- `assert 14 == 0`, 14 being the promoted provider's settings priority on the measured host. The 1 pre-existing pass is the negative control (a plan with no promotion is byte-identical after resume), which must pass on both sides. --- amplifier_app_cli/session_runner.py | 13 +- amplifier_app_cli/session_spawner.py | 248 ++++++++- ...est_resume_preserves_provider_promotion.py | 474 ++++++++++++++++++ 3 files changed, 732 insertions(+), 3 deletions(-) create mode 100644 tests/test_resume_preserves_provider_promotion.py diff --git a/amplifier_app_cli/session_runner.py b/amplifier_app_cli/session_runner.py index 2468aae1..ebaabdbc 100644 --- a/amplifier_app_cli/session_runner.py +++ b/amplifier_app_cli/session_runner.py @@ -550,10 +550,21 @@ async def spawn_capability( use_subprocess=use_subprocess, ) - async def resume_capability(sub_session_id: str, instruction: str) -> dict: + async def resume_capability( + sub_session_id: str, + instruction: str, + provider_preferences: list | None = None, + model_role: str | list[str] | None = None, + ) -> dict: + # Mirrors spawn_capability's provider_preferences: a delegate pinned + # to a provider at spawn must stay pinned on every subsequent leg. + # Both extras are optional, so a caller still invoking + # (sub_session_id, instruction) is unaffected. return await resume_sub_session( sub_session_id=sub_session_id, instruction=instruction, + provider_preferences=provider_preferences, + model_role=model_role, ) session.coordinator.register_capability("session.spawn", spawn_capability) diff --git a/amplifier_app_cli/session_spawner.py b/amplifier_app_cli/session_spawner.py index 6f8ee4a7..1585e739 100644 --- a/amplifier_app_cli/session_spawner.py +++ b/amplifier_app_cli/session_spawner.py @@ -9,6 +9,7 @@ import sys import time from pathlib import Path +from typing import Any from amplifier_core import AmplifierSession from amplifier_foundation import generate_sub_session_id @@ -907,11 +908,22 @@ async def child_spawn_capability( use_subprocess=use_subprocess, ) - async def child_resume_capability(sub_session_id: str, instruction: str) -> dict: + async def child_resume_capability( + sub_session_id: str, + instruction: str, + provider_preferences: list | None = None, + model_role: str | list[str] | None = None, + ) -> dict: + # Kept in step with child_spawn_capability above: a caller that can + # pin a provider at spawn must be able to pin the same one on every + # subsequent leg. Both extras are optional so an older caller that + # still invokes (sub_session_id, instruction) keeps working unchanged. return await resume_sub_session( sub_session_id=sub_session_id, instruction=instruction, parent_session=parent_session, + provider_preferences=provider_preferences, + model_role=model_role, ) child_session.coordinator.register_capability( @@ -1117,10 +1129,126 @@ async def _capture_completion(event: str, data: dict) -> HookResult: } +# --------------------------------------------------------------------------- +# Provider promotion across the resume boundary +# --------------------------------------------------------------------------- +# +# WHY THIS EXISTS (model_performance-rc0 / -n1i) +# +# A delegate spawned with model_role/provider_preferences gets its preferred +# provider promoted to priority 0 by apply_provider_preferences_with_resolution +# (see spawn_sub_session). That symbol used to appear EXACTLY ONCE in this +# file -- inside spawn_sub_session -- so the resume path could not rebuild the +# promotion after anything disturbed it. Combined with the credential refresh +# re-imposing settings `priority` (fixed separately, see +# narrow_overrides_to_secrets), a resumed leg silently re-resolved to the +# settings priority-0 provider: 39 of 66 delegate resumes changed model in a +# 2,078-session archive, 37 of them cheap -> expensive. +# +# The helpers below let resume REBUILD the promotion rather than merely +# preserve it, which additionally re-resolves the preference against the +# CURRENT provider set and gives the honest "could not honour it" signal. + + +def _normalize_model_role(model_role: str | list[str] | None) -> list[str]: + """Coerce a model_role declaration to the list form config stores.""" + if not model_role: + return [] + if isinstance(model_role, str): + return [model_role] + return [role for role in model_role if isinstance(role, str)] + + +def _coerce_provider_preferences(raw: Any) -> list: + """Coerce persisted/passed preferences to ProviderPreference objects. + + Accepts the dict form (how preferences are persisted in session metadata) + and already-constructed ProviderPreference objects (how a caller passes + them). Malformed entries are dropped with a warning rather than taking + down a resume -- a broken preference must not make a session unresumable. + """ + if not raw: + return [] + + from amplifier_foundation.spawn_utils import ProviderPreference + + coerced: list = [] + for entry in raw: + if isinstance(entry, ProviderPreference): + coerced.append(entry) + continue + if isinstance(entry, dict): + try: + coerced.append(ProviderPreference.from_dict(entry)) + except ValueError as e: + logger.warning( + "Skipping malformed provider preference %r: %s", entry, e + ) + continue + logger.warning("Skipping unusable provider preference %r", entry) + return coerced + + +def _provider_entry_keys(entry: dict) -> set[str]: + """Every name a preference may use to refer to this provider entry. + + Mirrors foundation's _build_provider_lookup: module id, the id-less short + name ("provider-anthropic" -> "anthropic"), and the instance id. + """ + module = entry.get("module") or "" + keys = {module, module.replace("provider-", "")} + instance_id = entry.get("id") + if instance_id: + keys.add(instance_id) + return {k for k in keys if k} + + +def _find_promoted_provider(providers: list, preferences: list) -> dict | None: + """Return the provider entry the preferences actually promoted, if any. + + Checks the OUTCOME (a preferred provider sitting at priority 0) rather + than trusting the return value of the apply call, so this stays honest + across foundation versions. + """ + wanted = {pref.provider for pref in preferences} + for entry in providers or []: + if not isinstance(entry, dict): + continue + if (entry.get("config") or {}).get("priority") != 0: + continue + if _provider_entry_keys(entry) & wanted: + return entry + return None + + +def _effective_provider(providers: list) -> dict | None: + """The entry the session will actually resolve: lowest priority number. + + Used to name what a leg LANDED on when a promotion could not be honoured. + Ties resolve to the first entry, matching mount-plan ordering. + """ + best: dict | None = None + best_priority: float | None = None + for entry in providers or []: + if not isinstance(entry, dict): + continue + priority = (entry.get("config") or {}).get("priority") + if not isinstance(priority, (int, float)) or isinstance(priority, bool): + continue + if best_priority is None or priority < best_priority: + best, best_priority = entry, priority + if best is None and providers: + first = providers[0] + return first if isinstance(first, dict) else None + return best + + async def resume_sub_session( sub_session_id: str, instruction: str, parent_session: AmplifierSession | None = None, + provider_preferences: list | None = None, + model_role: str | list[str] | None = None, ) -> dict: """Resume existing sub-session for multi-turn engagement. @@ -1130,6 +1258,16 @@ async def resume_sub_session( Args: sub_session_id: ID of existing sub-session to resume instruction: Follow-up instruction to execute + parent_session: Optional parent session (supplies the coordinator used + to resolve glob model patterns, and a working_dir fallback) + provider_preferences: Optional ordered list of ProviderPreference + objects (or their dict form), mirroring spawn_sub_session. When + omitted, preferences are recovered from the persisted session -- + first the agent overlay, then the mount plan -- so a caller that + has not yet been taught to thread them still keeps its promotion. + model_role: Optional model_role declaration to carry onto the resumed + leg, so its routing hook resolves the SAME role the spawn leg was + given rather than falling back to settings priority. Returns: Dict with "output" (response) and "session_id" (same ID) @@ -1337,6 +1475,92 @@ async def resume_sub_session( agent_name = metadata.get("agent_name", "unknown") trace_id = metadata.get("trace_id") + # --- Rebuild the provider promotion -------------------------------------- + # The spawn path applies model_role/provider_preferences here (see + # spawn_sub_session's "Apply provider preferences" block). Resume now does + # the same, so every leg of a delegate resolves the same way instead of + # inheriting whatever survived persistence. See the module-level comment + # above _normalize_model_role for the measured defect this closes. + _resume_agent_overlay = metadata.get("agent_overlay") or {} + + if model_role: + # Carry the caller's role onto the resumed leg so its routing hook + # resolves the SAME role the spawn leg was given. + merged_config = { + **merged_config, + "model_role": _normalize_model_role(model_role), + } + + # Precedence: what the caller threaded > the agent overlay as persisted > + # the persisted mount plan's own copy. The last two are recovery sources: + # they let a caller that still resumes with (session_id, instruction) keep + # its promotion, which is what makes this fix reach existing sessions. + _resume_preferences = _coerce_provider_preferences(provider_preferences) + _preferences_source = "caller" + if not _resume_preferences: + _resume_preferences = _coerce_provider_preferences( + _resume_agent_overlay.get("provider_preferences") + ) + _preferences_source = "agent_overlay" + if not _resume_preferences: + _resume_preferences = _coerce_provider_preferences( + merged_config.get("provider_preferences") + ) + _preferences_source = "persisted_config" + + _promotion_fallback: dict | None = None + if _resume_preferences: + from amplifier_foundation import apply_provider_preferences_with_resolution + + # parent_session may be absent (the root-registered resume capability + # passes none). apply_provider_preferences_with_resolution only needs a + # coordinator to expand GLOB model patterns and already degrades to + # "use the pattern as-is" when it cannot query one, so passing None is + # safe rather than fatal. + _resume_coordinator = ( + parent_session.coordinator if parent_session is not None else None + ) + merged_config = await apply_provider_preferences_with_resolution( + merged_config, _resume_preferences, _resume_coordinator + ) + + _promoted = _find_promoted_provider( + merged_config.get("providers") or [], _resume_preferences + ) + if _promoted is not None: + logger.debug( + "Sub-session %s: re-applied provider promotion on resume " + "(provider=%s, model=%s, preferences from %s)", + sub_session_id, + _promoted.get("module"), + (_promoted.get("config") or {}).get("default_model"), + _preferences_source, + ) + else: + # FAIL LOUD, DO NOT SILENTLY RE-RESOLVE. Silent re-resolution by + # settings priority is exactly the defect this fix exists to end; + # if the pin genuinely cannot be honoured, say so and name what + # the leg actually landed on. + _landed = _effective_provider(merged_config.get("providers") or []) + _promotion_fallback = { + "session_id": sub_session_id, + "agent_name": agent_name, + "reason": "preferred_provider_not_mounted", + "requested": [pref.to_dict() for pref in _resume_preferences], + "preferences_source": _preferences_source, + "provider": (_landed or {}).get("module"), + "model": (_landed or {}).get("config", {}).get("default_model"), + } + logger.warning( + "Sub-session %s: cannot honour provider preference(s) %s on " + "resume -- none is mounted in this session's plan. Falling " + "back to provider=%s model=%s.", + sub_session_id, + [pref.provider for pref in _resume_preferences], + _promotion_fallback["provider"], + _promotion_fallback["model"], + ) + # Sub-session resume creates fresh UX systems. Parent UX context (approval history, # display state) is not preserved across resume. This is acceptable because: # 1. Sub-sessions are typically short-lived agent delegations @@ -1495,11 +1719,22 @@ async def child_spawn_capability( use_subprocess=use_subprocess, ) - async def child_resume_capability(sub_session_id: str, instruction: str) -> dict: + async def child_resume_capability( + sub_session_id: str, + instruction: str, + provider_preferences: list | None = None, + model_role: str | list[str] | None = None, + ) -> dict: + # Kept in step with child_spawn_capability above: a caller that can + # pin a provider at spawn must be able to pin the same one on every + # subsequent leg. Both extras are optional so an older caller that + # still invokes (sub_session_id, instruction) keeps working unchanged. return await resume_sub_session( sub_session_id=sub_session_id, instruction=instruction, parent_session=child_session, + provider_preferences=provider_preferences, + model_role=model_role, ) child_session.coordinator.register_capability( @@ -1538,6 +1773,15 @@ async def child_resume_capability(sub_session_id: str, instruction: str) -> dict }, ) + # A promotion that could not be honoured is REPORTED, never silent. + # Emitted here rather than at merge time because the hook registry + # only exists once the session is initialized. The payload names the + # cause AND the provider/model the leg actually landed on, so an + # observer can tell "the pin was refused" from "the pin was wiped" -- + # the distinction the rc0 archive had no way to make. + if _promotion_fallback: + await hooks.emit("provider:fallback", _promotion_fallback) + # Re-register the agent's system prompt on resume. # # Mirrors the spawn path (see the "Inject agent's system instruction" diff --git a/tests/test_resume_preserves_provider_promotion.py b/tests/test_resume_preserves_provider_promotion.py new file mode 100644 index 00000000..f73f3e31 --- /dev/null +++ b/tests/test_resume_preserves_provider_promotion.py @@ -0,0 +1,474 @@ +"""Resumed sub-sessions must keep their spawn-time provider promotion. + +THE DEFECT (model_performance-rc0, confirmed on wire evidence) +-------------------------------------------------------------- +A sub-session spawned with ``model_role``/``provider_preferences`` carries +``priority: 0`` on the promoted provider in its persisted mount plan. +``resume_sub_session`` re-applied live ``settings.yaml`` provider overrides on +top of that plan to restore redacted credentials -- but the merge it used +(``merge_module_items`` -> ``deep_merge``, "overlay winning conflicts") +re-imposed EVERY settings key, ``config.priority`` included. The promotion +was overwritten and the resumed leg silently re-resolved to whatever sits at +settings priority 0. + +Measured over a 2,078-session archive: 66 delegate sessions contain a +``session:resume``; 39 (59%) changed model across the boundary; 37/39 +cheap -> expensive; every one reported ``basis: "priority"`` on BOTH sides +(a wipe, not a fallback). 0 of 179 root-session resumes were affected -- +a root plan has no promotion to lose. + +The wire fingerprint that identified the merge as the culprit: across the +boundary the promoted provider's config was identical in every key EXCEPT +those that ``settings.yaml`` declares. Keys present only in the preference's +own config (``enable_response_chaining``, ``prompt_cache_retention``) +survived; ``priority`` -- declared in settings -- did not. That is +``deep_merge(persisted, settings_override)`` semantics and nothing else. + +WHAT THIS MODULE COVERS +----------------------- +End-to-end behaviour of the real ``resume_sub_session()`` code path, for both +fixes: + +A. The drop site -- the resume credential refresh is narrowed to secret-bearing + keys, so it can no longer clobber ``priority`` (or any other per-candidate + config key). Isolated by + ``test_promotion_survives_with_no_recoverable_preferences``, which leaves no + preference anywhere for fix B to rebuild from. +B. The threading -- ``provider_preferences``/``model_role`` now reach the resume + path, so the promotion is REBUILT each leg rather than merely surviving. + +This module deliberately imports NO symbol introduced by either fix, so a +fail-before run against the pre-fix tree produces real assertion failures +rather than a collection error. Unit coverage of the narrowing helper itself +lives in ``test_narrow_overrides_to_secrets.py``. + +No API calls anywhere in this module. +""" + +from __future__ import annotations + +import logging +from unittest.mock import AsyncMock +from unittest.mock import MagicMock +from unittest.mock import patch + +import pytest +from amplifier_app_cli.session_spawner import resume_sub_session +from amplifier_app_cli.session_store import SessionStore + +pytestmark = pytest.mark.anyio + + +@pytest.fixture(scope="module") +def anyio_backend(): + """Configure anyio to use asyncio backend only.""" + return "asyncio" + + +# --------------------------------------------------------------------------- +# Fixtures modelled on the rc0 capture +# 20260901-rebaseline/runs/val-rb-oai-sol-xhigh-s1-01 +# .../0000000000000000-25443a97b60d4965_anchors-amp-dev-git-ops +# leg 1: luna priority 0 / sol priority 1 -> 13 x gpt-5.6-luna +# leg 2: luna priority 14 / sol priority 0 -> 25 x gpt-5.6-sol +# --------------------------------------------------------------------------- + +PROMOTED = "provider-luna" # the cheap tier the role promoted +SETTINGS_ZERO = "provider-sol" # what settings puts at priority 0 + + +def _persisted_child_providers() -> list[dict]: + """The child's mount plan as spawn built it, then redaction persisted it. + + ``priority: 0`` on luna is the promotion; sol was demoted to 1. + ``api_key`` is the sentinel redact_secrets() writes to disk. + """ + return [ + { + "module": PROMOTED, + "config": { + "api_key": "[REDACTED]", + "default_model": "gpt-5.6-luna", + "priority": 0, + "reasoning_effort": "medium", + # Present ONLY in the preference's own config -- never in + # settings.yaml. These survived the wipe in the capture and + # are what proved the child's config was merged, not rebuilt. + "enable_response_chaining": "auto", + "prompt_cache_retention": "in_memory", + }, + }, + { + "module": SETTINGS_ZERO, + "config": { + "api_key": "[REDACTED]", + "default_model": "gpt-5.6-sol", + "priority": 1, + }, + }, + ] + + +def _live_settings_overrides() -> list[dict]: + """Live settings.yaml: real keys, plus the priorities that did the damage. + + ``reasoning_effort`` differs from the child's persisted value on purpose: + rc0 recorded that drift as INFERRED-NOT-CONFIRMED because the capture had + the same effort on both sides. Differing values settle it here. + """ + return [ + { + "module": PROMOTED, + "config": { + "api_key": "sk-live-luna", + "priority": 14, + "reasoning_effort": "high", + }, + }, + { + "module": SETTINGS_ZERO, + "config": { + "api_key": "sk-live-sol", + "priority": 0, + }, + }, + ] + + +def _by_module(providers: list[dict], module: str) -> dict: + return next(p for p in providers if p["module"] == module) + + +# --------------------------------------------------------------------------- +# Fix B -- threading role/preferences through resume +# --------------------------------------------------------------------------- + + +def _base_metadata(session_id: str, **overrides) -> dict: + metadata = { + "session_id": session_id, + "parent_id": "parent-123", + "agent_name": "git-ops", + "config": { + "session": {"orchestrator": "loop-basic", "context": "context-simple"}, + "providers": _persisted_child_providers(), + }, + "working_dir": "/test/project", + "self_delegation_depth": 0, + } + metadata.update(overrides) + return metadata + + +class _FakeContext: + def __init__(self) -> None: + self.messages: list[dict] = [] + + async def set_system_prompt_factory(self, factory) -> None: + return None + + async def add_message(self, message: dict) -> None: + self.messages.append(message) + + async def get_messages(self) -> list[dict]: + return self.messages + + +class _RecordingHooks: + """Minimal stand-in for the hook registry: records every emit().""" + + def __init__(self) -> None: + self.emitted: list[tuple[str, dict]] = [] + self.registered: list[str] = [] + + async def emit(self, event: str, data: dict) -> None: + self.emitted.append((event, data)) + + def register(self, event: str, handler, priority: int = 0, name: str = ""): + self.registered.append(event) + return lambda: None + + +async def _run_resume( + session_id: str, + *, + provider_overrides: list[dict] | None = None, + **resume_kwargs, +) -> tuple[dict, _RecordingHooks]: + """Drive the real resume_sub_session() and capture the mounted config. + + Returns (config handed to AmplifierSession, hooks that recorded emits). + """ + captured: dict = {} + hooks = _RecordingHooks() + fake_context = _FakeContext() + + def mock_get(name): + if name == "context": + return fake_context + if name == "hooks": + return hooks + return None + + mock_coordinator = MagicMock() + mock_coordinator.register_capability = MagicMock() + mock_coordinator.get_capability = MagicMock(return_value=None) + mock_coordinator.get = MagicMock(side_effect=mock_get) + mock_coordinator.mount = AsyncMock() + + mock_session = MagicMock() + mock_session.coordinator = mock_coordinator + mock_session.initialize = AsyncMock() + mock_session.execute = AsyncMock(return_value="response") + mock_session.cleanup = AsyncMock() + + def _capture(*args, **kwargs): + captured["config"] = kwargs.get("config") + return mock_session + + mock_settings = MagicMock() + mock_settings.get_provider_overrides = MagicMock( + return_value=provider_overrides if provider_overrides is not None else [] + ) + mock_settings.get_config_overrides = MagicMock(return_value={}) + mock_settings.get_notification_hook_overrides = MagicMock(return_value=[]) + + with ( + patch( + "amplifier_app_cli.session_spawner.AmplifierSession", side_effect=_capture + ), + patch("amplifier_app_cli.lib.settings.AppSettings", return_value=mock_settings), + patch("amplifier_app_cli.ui.CLIApprovalSystem"), + patch("amplifier_app_cli.ui.CLIDisplaySystem"), + patch("amplifier_app_cli.paths.create_foundation_resolver"), + ): + await resume_sub_session(session_id, "follow-up", **resume_kwargs) + + return captured["config"], hooks + + +class TestResumeRebuildsPromotion: + """The resume path must re-apply the promotion, not merely preserve it.""" + + async def test_resumed_leg_keeps_its_model_role_promotion( + self, tmp_path, monkeypatch + ): + """THE headline regression test. FAILS BEFORE THE FIX. + + Before: the settings merge wiped ``priority: 0`` and nothing on the + resume path could rebuild it -- ``apply_provider_preferences_with_ + resolution`` was reachable only from spawn. The resumed leg resolved + to the settings priority-0 provider (sol), exactly as captured. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-keeps-promotion" + metadata = _base_metadata( + session_id, + agent_overlay={ + "model_role": ["fast", "general"], + "provider_preferences": [ + {"provider": "luna", "model": "gpt-5.6-luna"}, + ], + }, + ) + store.save(session_id, [{"role": "user", "content": "hi"}], metadata) + + config, _ = await _run_resume( + session_id, provider_overrides=_live_settings_overrides() + ) + + promoted = _by_module(config["providers"], PROMOTED) + assert promoted["config"]["priority"] == 0, ( + "A resumed delegate must resolve to the SAME provider its spawn " + "leg did. Landing on the settings priority-0 provider is the rc0 " + "defect: 39/66 delegate resumes changed model, 37 cheap->expensive." + ) + assert promoted["config"]["default_model"] == "gpt-5.6-luna" + + async def test_promotion_survives_with_no_recoverable_preferences( + self, tmp_path, monkeypatch + ): + """Isolates FIX A through the production path. FAILS BEFORE FIX A. + + No preferences exist anywhere -- not threaded, not in the agent + overlay, not in the mount plan -- so fix B cannot rebuild anything. + The persisted promotion must survive the credential refresh on its + own, and the credential must still be refreshed. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-fix-a-isolated" + store.save(session_id, [], _base_metadata(session_id)) + + config, _ = await _run_resume( + session_id, provider_overrides=_live_settings_overrides() + ) + + promoted = _by_module(config["providers"], PROMOTED) + assert promoted["config"]["priority"] == 0, ( + "The persisted promotion must survive the resume credential " + "refresh even when no preference can be recovered to rebuild it." + ) + assert _by_module(config["providers"], SETTINGS_ZERO)["config"]["priority"] == 1 + assert promoted["config"]["reasoning_effort"] == "medium" + # The refresh must still do the job it exists for. + assert promoted["config"]["api_key"] == "sk-live-luna" + + async def test_explicit_preferences_argument_is_honoured( + self, tmp_path, monkeypatch + ): + """The threaded argument wins over anything persisted. + + This is the hop the caller (tool-delegate's resume path) gains: it + can now pass the same preferences it passes at spawn. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-explicit-prefs" + store.save(session_id, [], _base_metadata(session_id)) + + from amplifier_foundation.spawn_utils import ProviderPreference + + config, _ = await _run_resume( + session_id, + provider_overrides=_live_settings_overrides(), + provider_preferences=[ + ProviderPreference(provider="luna", model="gpt-5.6-luna") + ], + ) + + assert _by_module(config["providers"], PROMOTED)["config"]["priority"] == 0 + + async def test_preferences_recovered_from_persisted_mount_plan( + self, tmp_path, monkeypatch + ): + """Sessions saved with no agent_overlay still recover their promotion. + + merge_configs copies the agent overlay's top-level keys into the + merged mount plan, so ``provider_preferences`` is present there too -- + which is what the rc0 capture observed ("still luna ... simply never + consulted again"). + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-prefs-from-config" + metadata = _base_metadata(session_id) + metadata["config"]["provider_preferences"] = [ + {"provider": "luna", "model": "gpt-5.6-luna"} + ] + store.save(session_id, [], metadata) + + config, _ = await _run_resume( + session_id, provider_overrides=_live_settings_overrides() + ) + + assert _by_module(config["providers"], PROMOTED)["config"]["priority"] == 0 + + async def test_preference_config_is_reasserted_on_resume( + self, tmp_path, monkeypatch + ): + """Per-candidate keys carried by the preference are re-applied. + + Settles rc0 section 4.6 from the other direction: the preference's own + ``reasoning_effort`` -- not settings' -- governs the resumed leg. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-pref-config" + metadata = _base_metadata( + session_id, + agent_overlay={ + "provider_preferences": [ + { + "provider": "luna", + "model": "gpt-5.6-luna", + "config": {"reasoning_effort": "low"}, + } + ] + }, + ) + store.save(session_id, [], metadata) + + config, _ = await _run_resume( + session_id, provider_overrides=_live_settings_overrides() + ) + + promoted = _by_module(config["providers"], PROMOTED) + assert promoted["config"]["reasoning_effort"] == "low" + + async def test_model_role_is_written_into_the_resumed_config( + self, tmp_path, monkeypatch + ): + """A threaded model_role reaches the resumed session's config. + + The resumed leg's routing hook resolves roles from config; without + this the role the delegate was spawned with never reaches it. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-model-role" + store.save(session_id, [], _base_metadata(session_id)) + + config, _ = await _run_resume(session_id, model_role="fast") + + assert config["model_role"] == ["fast"] + + async def test_unhonourable_promotion_emits_a_fallback_event( + self, tmp_path, monkeypatch, caplog + ): + """Acceptance criterion: name the cause, do not silently re-resolve. + + When the pinned provider is not in the mount plan at all, the resumed + leg still has to run on something -- but it must SAY so, naming the + cause and the provider/model it actually landed on. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-fallback-event" + metadata = _base_metadata( + session_id, + agent_overlay={ + "provider_preferences": [ + {"provider": "nonexistent", "model": "no-such-model"} + ] + }, + ) + store.save(session_id, [], metadata) + + with caplog.at_level(logging.WARNING): + _, hooks = await _run_resume(session_id) + + fallbacks = [d for name, d in hooks.emitted if name == "provider:fallback"] + assert fallbacks, ( + "An unhonourable promotion on resume must emit a named fallback " + "event rather than silently re-resolving by settings priority." + ) + payload = fallbacks[0] + assert payload["reason"] == "preferred_provider_not_mounted" + assert payload["requested"] == [ + {"provider": "nonexistent", "model": "no-such-model"} + ] + # It must name what the leg actually landed on. + assert payload["provider"] == PROMOTED + assert payload["model"] == "gpt-5.6-luna" + + async def test_no_preferences_leaves_the_plan_byte_identical( + self, tmp_path, monkeypatch + ): + """Default behaviour is unchanged when nothing was ever promoted. + + This is the negative control that mirrors rc0's own: 0 of 179 root + resumes were affected, because a plan with no promotion has nothing + to preserve and nothing to rebuild. + """ + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "test-resume-no-prefs" + metadata = _base_metadata(session_id) + store.save(session_id, [], metadata) + + config, hooks = await _run_resume(session_id) + + assert config["providers"] == _persisted_child_providers() + assert not [n for n, _ in hooks.emitted if n == "provider:fallback"] From 57accd9e9a3501479a42696b4a993b455d08f1f2 Mon Sep 17 00:00:00 2001 From: amplifier-lane Date: Wed, 2 Sep 2026 17:06:18 -0700 Subject: [PATCH 3/3] docs(lane): DONE-NOTE for model_performance-n1i Lane bookkeeping for the two fix commits above: the file:line sites rc0 identified, the fail-before/pass-after evidence, the root-resume and credential-refresh verification, decisions taken without escalation, and what remains open. Kept as its own commit so the two fix commits stay clean and this is trivially droppable before merge. --- .../lanes/n1i-resume-thread-role/DONE-NOTE.md | 338 ++++++++++++++++++ 1 file changed, 338 insertions(+) create mode 100644 docs/lanes/n1i-resume-thread-role/DONE-NOTE.md diff --git a/docs/lanes/n1i-resume-thread-role/DONE-NOTE.md b/docs/lanes/n1i-resume-thread-role/DONE-NOTE.md new file mode 100644 index 00000000..e65743ce --- /dev/null +++ b/docs/lanes/n1i-resume-thread-role/DONE-NOTE.md @@ -0,0 +1,338 @@ +# DONE-NOTE — `model_performance-n1i` + +**FIX: narrow the resume credential refresh to secrets, and thread +`model_role`/`provider_preferences` through the delegate resume path** + +Lane `n1i-resume-thread-role` · repo `microsoft/amplifier-app-cli` · +branch `lane/n1i-resume-thread-role` · 2026-09-02 · **spend $0.00** + +Implements the root cause diagnosed in `model_performance-rc0` +(`ai-notes/w3-rc0-resume-role-loss/FINDINGS.md`). That diagnosis was CONFIRMED +ON WIRE EVIDENCE and was **not** re-litigated here; this lane implemented it. + +--- + +## 1. DELIVERABLES + +| # | Deliverable | Status | +|---|---|---| +| 1 | DRAFT PR on origin, branch `lane/n1i-resume-thread-role`, two fixes as separate commits, tests green | **DONE** | +| 2 | Fail-before/pass-after test proving a resumed delegate retains its `model_role` promotion | **DONE** (§5) | +| 3 | Explicit confirmation root-session resume + credential refresh are unaffected, with how verified | **DONE** (§4) | +| 4 | DONE-NOTE.md in the PR body quoting the file:line sites rc0 identified | **DONE** (this file) | + +**Commits:** + +``` +fix(resume): narrow the sub-session credential refresh to secrets only +fix(resume): thread model_role/provider_preferences through the delegate resume path +``` + +--- + +## 2. COMMIT A — the DROP SITE, narrowed to secrets + +`amplifier_app_cli/runtime/config.py`, `amplifier_app_cli/session_spawner.py`, +`tests/test_narrow_overrides_to_secrets.py` + +### The sites rc0 identified, quoted + +Pre-fix line numbers are rc0's, taken against +`openai-evals-team-ci/amplifier-app-cli @ ed89a9f`. This lane's worktree is at +`0d93352`, a later commit that shifted the block down ~200 lines; current +numbers are given alongside. + +**`session_spawner.py:1005-1010`** (here `:1214-1219` pre-fix), inside +`resume_sub_session`: + +```python + _live_provider_overrides = _live_settings.get_provider_overrides() + if _live_provider_overrides: + _refreshed_providers = _apply_provider_overrides( + merged_config["providers"], _live_provider_overrides + ) +``` + +**`runtime/config.py:564`** — `_apply_provider_overrides`: + +```python + merged = merge_module_items(provider, override_map[key]) +``` + +**`lib/merge_utils.py:149-152`** — where the promotion is actually lost: + +```python + if key == "config" and key in merged: + # Deep merge configs + if isinstance(merged["config"], dict) and isinstance(value, dict): + merged["config"] = deep_merge(merged["config"], value) +``` + +**`lib/merge_utils.py:64-65`**: + +```python +def deep_merge(base: dict[str, Any], overlay: dict[str, Any]) -> dict[str, Any]: + """Deep merge two dicts, with overlay winning conflicts. +``` + +`base` = the child's persisted provider config (`priority: 0`, installed at +spawn by `spawn_utils.py:772-773`); `overlay` = the settings override +(`priority: 14` for luna on the measured host). **Overlay wins.** + +The enclosing comment block (`session_spawner.py:971-991`) scopes this refresh +to *secrets*. `priority` is not a secret — collateral damage from the +metadata-redaction security fix. + +### The change + +New `narrow_overrides_to_secrets()` in `runtime/config.py` reduces a settings +override to the keys `redact_secrets()` actually redacted, reusing +`amplifier_core.utils.truncate.SENSITIVE_KEYS` **so the two directions can +never drift apart** — if redaction learns a new secret key, the refresh learns +it too, with no second list to maintain. + +Pruning rules, and why each is what it is: + +- **dict** — a key whose *name* is sensitive is kept outright; otherwise + recurse, keeping the key only if something secret survives beneath it + (covers `config.auth.token`). +- **list** — kept **whole** if any element carries a secret, else dropped. + `deep_merge` *replaces* lists rather than merging them, so a partially + pruned list would silently truncate the merged result. All-or-nothing is the + only safe choice. +- **identity keys** — `module` and `id` carried through so the override still + matches its target; **every other top-level key dropped**, so a settings + override cannot rewrite `source` at resume time either. + +**Scope, deliberately narrow:** applied at the RESUME call site only. Root and +fresh config assembly (`resolve_bundle_config`) still merges overrides in +full — there settings *are* the intended source of truth and there is no +persisted child promotion to protect. Verified: `narrow_overrides_to_secrets` +has exactly one call site (§4). + +### rc0 §4.6 — the INFERRED-NOT-CONFIRMED sub-claim, now settled + +rc0 could not observe `reasoning_effort` drift because the capture had `high` +on both sides. `merge_utils.py:152` merges *every* settings key, so the drift +was structurally possible but unobservable. +`test_per_candidate_config_keys_survive` uses a preference effort of `medium` +against a settings effort of `high` and asserts the child's own value wins. +**Verdict upgraded: CONFIRMED, and fixed.** + +--- + +## 3. COMMIT B — the THREADING + +`amplifier_app_cli/session_spawner.py`, `amplifier_app_cli/session_runner.py`, +`tests/test_resume_preserves_provider_promotion.py` + +### The gap, as rc0 §2.2 mapped it + +| hop | spawn path | resume path | +|---|---|---| +| tool-delegate call site | `__init__.py:1509` — prefs + role | `__init__.py:1444` — neither | +| capability invocation | `__init__.py:1771` `spawn_fn(… provider_preferences=…)` | `__init__.py:2084` `resume_fn(sub_session_id=…, instruction=…)` | +| app-cli capability | `session_spawner.py:715,730` | `session_spawner.py:736,1253` — **2 args** | +| app-cli implementation | `session_spawner.py:231,241` `spawn_sub_session(… provider_preferences …)` | `session_spawner.py:923-926` `resume_sub_session(sub_session_id, instruction, parent_session)` | +| promotion applied? | `session_spawner.py:417-421` → `apply_provider_preferences_with_resolution` | **never** | + +The decisive grep: `apply_provider_preferences_with_resolution` appeared +**exactly once** in `session_spawner.py`, inside `spawn_sub_session`. + +### The change + +- `resume_sub_session(..., provider_preferences=None, model_role=None)` +- both `child_resume_capability` closures (`session_spawner.py`) and + `resume_capability` (`session_runner.py:553`) gain the same two optional + keyword arguments, matching their `child_spawn_capability` siblings +- the resume path now calls `apply_provider_preferences_with_resolution` + exactly as `spawn_sub_session` does + +**Preference precedence:** threaded by the caller > persisted agent overlay > +persisted mount plan's own copy. + +The two recovery sources are the load-bearing design decision. rc0 §3 recorded +that both `provider_preferences` and `model_role` were **still present in the +resumed session's config and simply never consulted again**. Recovering them +means the fix reaches existing sessions and existing callers *without* the +foundation-side change; the threaded argument is what a caller gains once it is +taught to pass them. + +**Fallback is named, never silent.** Per the rc0 acceptance criteria: when no +preferred provider is mounted, resume emits `provider:fallback` carrying +`reason`, the requested preferences, `preferences_source`, and the +provider/model the leg actually landed on. Promotion success is verified from +the **outcome** (a preferred provider sitting at `priority: 0`), not from the +apply call's return value, so the check stays honest across foundation +versions. + +### NOT IN THIS REPO — stated plainly + +The matching change in `microsoft/amplifier-foundation` +(`modules/tool-delegate/.../__init__.py:1997` `_resume_existing_session`, its +call site at `:1444`, and `resume_fn` at `:2084`) is what makes the *caller* +pass these through. This lane owns `amplifier-app-cli` only, so that repo is +untouched. The signatures added here are additive and backward-compatible +precisely so the two can land independently — and the recovery sources above +mean the measured defect is fixed either way. + +--- + +## 4. DELIVERABLE 3 — root resume and credential refresh are unaffected + +Verification script output, run against both commits: + +``` +### 1. narrow_overrides_to_secrets call sites (must be exactly 1, in resume_sub_session) +amplifier_app_cli/session_spawner.py:1370: _live_provider_overrides = narrow_overrides_to_secrets( + +### 3. resume_sub_session importers (root resume must not be among them) +amplifier_app_cli/session_runner.py:519: from .session_spawner import resume_sub_session +amplifier_app_cli/session_runner.py:563: return await resume_sub_session( + +### 4. root-session resume path (commands/session.py) never imports session_spawner +0 + +### 5. pre-existing credential-refresh suite +15 passed + +### 6. new suites +21 passed + +### 7. full suite +1594 passed, 1 skipped, 13 deselected, 1 xfailed +``` + +**Root-session resume — how it was verified.** `resume_sub_session` has exactly +one importer in the whole package: `session_runner.py:519`, inside +`register_session_spawning`, which registers it as the **`session.resume` +capability** — that is *root-resumes-a-SUB-session*, not *root resumes itself*. +Root-session resume runs through `commands/session.py::_prepare_resume_context` +→ `resolve_config` → `resolve_bundle_config`, which imports `session_spawner` +**zero** times (check 4) and whose override merging this change does not touch +(check 1: the narrowing has a single call site, inside `resume_sub_session`). +This matches rc0's own strongest negative control — **0 of 179 root resumes +affected** — from the opposite direction: rc0 *measured* that roots were never +hit; this shows *structurally* that they still cannot be. + +**Credential refresh — how it was verified.** The pre-existing suites +`test_resume_credential_refresh.py` (5) and `test_resume_redaction_guard.py` +(10) pass unmodified. Two new tests assert the refresh still does its job on +the narrowed path: `test_credentials_are_still_refreshed` (unit) and +`test_promotion_survives_with_no_recoverable_preferences` (through the real +`resume_sub_session`, asserting `api_key == "sk-live-luna"` alongside +`priority == 0`). + +**Default behaviour otherwise byte-identical.** +`test_no_preferences_leaves_the_plan_byte_identical` resumes a plan carrying no +promotion and asserts `config["providers"] == _persisted_child_providers()` +with no `provider:fallback` emitted. It is the one test that **passes on both +sides of the fix** — the negative control. + +--- + +## 5. FAIL-BEFORE / PASS-AFTER + +**Fail-before, both fixes reverted** (`git stash push amplifier_app_cli/`, run, +pop) — genuine assertion failures, not a collection error, because the +behavioural module deliberately imports no symbol introduced by either fix: + +``` +7 failed, 1 passed + +test_resumed_leg_keeps_its_model_role_promotion +E AssertionError: A resumed delegate must resolve to the SAME provider its + spawn leg did. Landing on the settings priority-0 provider is the rc0 + defect: 39/66 delegate resumes changed model, 37 cheap->expensive. +E assert 14 == 0 +``` + +`14` is the promoted provider's **byte-exact settings priority** from the +measured host (`~/.amplifier/settings.yaml` declares `luna: priority: 14`). +The unit test reproduces the wire signature exactly. + +The 1 pre-existing pass is the negative control described above. + +**Fail-before for commit B alone** (commit A applied, B reverted): `4 failed, 4 +passed`. The 4 that already pass are the ones commit A alone fixes — which is +what the item predicted ("This alone resolves the observed defect"). The 4 that +still fail are commit B's distinct value: explicit preference threading, +preference-config re-assertion, `model_role` threading, and the +`provider:fallback` event. + +**Pass-after:** 21/21 across both new modules; **1594 passed, 1 skipped, 1 +xfailed** for the full suite. No API calls in any new test; the whole suite +runs in ~8 s. + +**Lint:** `ruff check` reports **14 errors before and 14 after** — all +pre-existing, none introduced. `ruff format` applied to the touched files. + +--- + +## 6. DECISIONS TAKEN WITHOUT ESCALATION + +Per the lane rules ("no waiting on any human decision: choose, record, continue"): + +1. **The hook refresh in the same block is NOT narrowed.** The item asked to + "guard against the same class in the sibling paths". For providers the guard + is the code change; for hooks it is a documented boundary, and the reasoning + is recorded inline at the site. Two reasons: (a) nothing in a hook entry + carries per-session *resolution* state — the provider wipe mattered because + `config.priority` decides which model a leg runs on, and a hook has no + analogue; (b) `get_notification_hook_overrides()` legitimately **appends** + hooks absent from the persisted plan, so narrowing to secrets would append + them stripped of `enabled`/`topic`, breaking notifications on resumed + sub-sessions to fix a defect nobody has observed. The same over-reach *is* + structurally possible for a hook whose config an agent overlay customised; + **that needs its own evidence, not a speculative change.** Flagged as a known + open edge rather than silently fixed or silently ignored. +2. **`_apply_provider_overrides` / `_apply_hook_overrides` / the tool override + merge themselves are unchanged.** Root config assembly depends on their full + merge semantics. Narrowing them at source would have changed root behaviour + to fix a resume-only defect. +3. **The fallback event is a new string, `provider:fallback`**, matching the + `provider:` namespace of `provider:resolve` / `provider:retry` / + `provider:error` in `amplifier_core.events`. No existing constant covered + "the pin could not be honoured". +4. **This DONE-NOTE is committed under `docs/lanes/n1i-resume-thread-role/`** + as its own third commit, so the two fix commits stay clean and the lane + bookkeeping is trivially droppable before any merge. (First choice was to + keep it outside the repo entirely; the lane's write sandbox is the repo, and + the deliverable requires it committed under this lane's own directory.) +5. **Test module split.** Unit coverage of the new helper lives in + `test_narrow_overrides_to_secrets.py`; the behavioural module imports **no** + new symbol, so a fail-before run produces real assertion failures rather than + an `ImportError` at collection. The first draft did not do this and its + fail-before run was a worthless collection error — corrected before commit. + +--- + +## 7. SPEND + +**$0.00**, against a $0 authority. No API calls, no DTU, no containers, no +infrastructure created — so nothing to register in the infra ledger and nothing +to tear down. Every test runs offline against mocked sessions. + +--- + +## 8. WHAT REMAINS OPEN + +1. **`microsoft/amplifier-foundation` tool-delegate is untouched** (§3). Until + it threads `provider_preferences`/`raw_model_role` into `resume_fn`, the + promotion is rebuilt from the *persisted* preference rather than a freshly + routed one. Functionally equivalent for the measured defect; it does mean a + routing-matrix change between two legs of the same delegate is not picked up + mid-session. +2. **The hook-refresh over-reach** (§6.1) — same class, unmeasured, deliberately + left with a named comment instead of a speculative fix. +3. **`amplifier-bundle-routing-matrix`'s `role_pin.py`** (rc0 §5) should stay as + defense-in-depth. This fix is upstream of it; it is now belt-and-braces + rather than the only guard. +4. **Not computed:** the dollar delta of the 402 drifted requests. rc0 recorded + this as NOT COMPUTED and this lane did not change that — it needs per-request + usage joined to per-model pricing, and would not have changed any decision + here. +5. **`00-what-we-know.md` §2c's "symmetric confounder" justification** should + still be amended per rc0 §4.5 — this bias is asymmetric and exaggerates the + measured cost spread between cells. That is a docs change in the evals repo, + outside this lane's paths.