diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 54331a32..0cf4974c 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import copy import logging import os import re @@ -26,6 +27,8 @@ logger = logging.getLogger(__name__) +_REDACTION_SENTINEL = "[REDACTED]" + async def resolve_bundle_config( bundle_name: str, @@ -650,6 +653,146 @@ def _prune_to_secret_keys(value: Any) -> Any | None: return None +def _contains_redacted_secret(value: Any) -> bool: + """Return whether a structure contains a persisted redacted secret leaf.""" + if isinstance(value, dict): + return any( + ( + isinstance(key, str) + and key.lower() in SENSITIVE_KEYS + and child == _REDACTION_SENTINEL + ) + or _contains_redacted_secret(child) + for key, child in value.items() + ) + if isinstance(value, list): + return any(_contains_redacted_secret(child) for child in value) + return False + + +def _stable_list_identity(entry: Any, key: str) -> str | None: + """Return a usable non-secret list-entry identity, if one is present.""" + if not isinstance(entry, dict): + return None + value = entry.get(key) + return value if isinstance(value, str) and value else None + + +def _nonsecret_structure(value: Any) -> Any: + """Copy a value's non-secret structure for conservative list matching.""" + if isinstance(value, dict): + return { + key: _nonsecret_structure(child) + for key, child in value.items() + if not (isinstance(key, str) and key.lower() in SENSITIVE_KEYS) + } + if isinstance(value, list): + return [_nonsecret_structure(child) for child in value] + return value + + +def _matching_live_list_entry( + persisted_entry: Any, persisted: list[Any], live: list[Any] +) -> tuple[Any | None, str | None]: + """Find one unambiguous live counterpart without relying on list position.""" + if not isinstance(persisted_entry, dict): + return None, "entry is not a mapping" + + identity_failures: list[str] = [] + for identity_key in ("id", "name"): + identity = _stable_list_identity(persisted_entry, identity_key) + if identity is None: + continue + persisted_count = sum( + _stable_list_identity(entry, identity_key) == identity + for entry in persisted + ) + matches = [ + entry + for entry in live + if _stable_list_identity(entry, identity_key) == identity + ] + if persisted_count == 1 and len(matches) == 1: + return matches[0], None + if persisted_count > 1 or len(matches) > 1: + identity_failures.append(f"ambiguous {identity_key}") + else: + identity_failures.append(f"missing {identity_key}") + + # A structural comparison is safe only for an entirely identity-less list: + # an id/name mismatch is evidence that these entries are not interchangeable. + has_any_identity = any( + _stable_list_identity(entry, identity_key) is not None + for entry in [*persisted, *live] + for identity_key in ("id", "name") + ) + if identity_failures: + return None, identity_failures[0] + if has_any_identity: + return None, "missing stable identity" + + matches = [ + entry + for entry in live + if _nonsecret_structure(entry) == _nonsecret_structure(persisted_entry) + ] + if len(matches) == 1: + return matches[0], None + if len(matches) > 1: + return None, "ambiguous non-secret structure" + return None, "no matching non-secret structure" + + +def restore_redacted_secret_values(persisted: Any, live: Any) -> Any: + """Copy ``persisted``, replacing only matching redacted secret leaves. + + Resuming a child starts with its redacted persisted mount plan, not a new + bundle plan. A normal deep merge is therefore wrong: it lets current + settings rewrite unrelated child routing and module settings. This small + inverse of redaction preserves the persisted shape and only restores a + value when all of these are true: + + * the persisted value is exactly the session-store redaction sentinel; + * its key is in core's authoritative ``SENSITIVE_KEYS`` set; and + * the same key exists at the same path in usable live configuration. + + Lists are never extended or replaced. Entries are matched by a unique + non-secret ``id`` or ``name``; identity-less lists must have exactly one + matching non-secret structure. This deliberately preserves a child's list + order, entries, and any child-specific unredacted credentials without + assigning a credential from a reordered live list to the wrong URL. + """ + if isinstance(persisted, dict): + result = copy.deepcopy(persisted) + live_dict = live if isinstance(live, dict) else {} + for key, value in persisted.items(): + live_value = live_dict.get(key) + if ( + isinstance(key, str) + and key.lower() in SENSITIVE_KEYS + and value == _REDACTION_SENTINEL + and live_value not in (None, "", _REDACTION_SENTINEL) + ): + result[key] = copy.deepcopy(live_value) + elif isinstance(value, (dict, list)) and isinstance( + live_value, type(value) + ): + result[key] = restore_redacted_secret_values(value, live_value) + return result + if isinstance(persisted, list): + result = copy.deepcopy(persisted) + if not isinstance(live, list): + return result + for index, value in enumerate(persisted): + if not isinstance(value, (dict, list)): + continue + live_value, _ = _matching_live_list_entry(value, persisted, live) + if live_value is not None and isinstance(live_value, type(value)): + result[index] = restore_redacted_secret_values(value, live_value) + return result + return copy.deepcopy(persisted) + + def narrow_overrides_to_secrets( 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 dc5d7a59..aee1c24f 100644 --- a/amplifier_app_cli/session_spawner.py +++ b/amplifier_app_cli/session_spawner.py @@ -4,6 +4,7 @@ """ import copy +import inspect import logging import os import sys @@ -12,6 +13,7 @@ from typing import Any from amplifier_core import AmplifierSession +from amplifier_core.utils.truncate import SENSITIVE_KEYS from amplifier_foundation import generate_sub_session_id from amplifier_foundation import bridge_child_cost from amplifier_foundation import RUNTIME_SKILL_OVERLAY_CAPABILITY @@ -674,7 +676,9 @@ def _filter_hooks( _REDACTION_SENTINEL = "[REDACTED]" -def _find_redacted_values(value: object, path: str = "") -> list[str]: +def _find_redacted_values( + value: object, path: str = "", is_sensitive_value: bool = False +) -> list[str]: """Recursively collect dotted/bracketed paths still holding the redaction sentinel. Used at resume time (see resume_sub_session's credential refresh) to detect @@ -695,11 +699,17 @@ def _find_redacted_values(value: object, path: str = "") -> list[str]: found: list[str] = [] if isinstance(value, dict): for key, sub_value in value.items(): - found.extend(_find_redacted_values(sub_value, f"{path}.{key}")) + found.extend( + _find_redacted_values( + sub_value, + f"{path}.{key}", + isinstance(key, str) and key.lower() in SENSITIVE_KEYS, + ) + ) elif isinstance(value, list): for index, item in enumerate(value): found.extend(_find_redacted_values(item, f"{path}[{index}]")) - elif value == _REDACTION_SENTINEL: + elif is_sensitive_value and value == _REDACTION_SENTINEL: found.append(path or "") return found @@ -1038,6 +1048,11 @@ async def spawn_sub_session( merged_config, hook_inheritance, agent_hook_modules ) + # Keep caller intent separate from the agent-authored fallback. The + # merged mount plan gets the actual effective chain below, while metadata + # records only an explicit caller override for cold resume precedence. + _caller_provider_preferences = _serialize_provider_preferences(provider_preferences) + # Defense-in-depth: read routing-resolved provider_preferences from agent config # when no explicit preferences were passed by the caller. # The routing hook (hooks-routing) writes provider_preferences into agent configs @@ -1061,13 +1076,44 @@ async def spawn_sub_session( len(provider_preferences), ) - # Apply provider preferences if specified (ordered fallback chain) + # Serialize the same complete ordered chain that resolution sees before + # mounting and persisting. In particular, do not leave an older + # agent/default chain at config["provider_preferences"] when a caller + # supplied a different preference. + _effective_provider_preferences = _serialize_provider_preferences( + provider_preferences + ) + if _effective_provider_preferences: + merged_config = { + **merged_config, + "provider_preferences": _effective_provider_preferences, + } + else: + merged_config = dict(merged_config) + merged_config.pop("provider_preferences", None) + + # Apply provider preferences if specified (ordered fallback chain). + # Newer Foundation records every attempt in ``_resolution_diagnostics``; + # it suppresses its per-attempt warning when a sink is supplied so the + # CLI can emit one final, truthful diagnostic after the full chain fails. + _spawn_preference_failure: dict[str, Any] | None = None if provider_preferences: - from amplifier_foundation import apply_provider_preferences_with_resolution - - merged_config = await apply_provider_preferences_with_resolution( + merged_config, _resolution_diagnostics = await _apply_provider_preferences( merged_config, provider_preferences, parent_session.coordinator ) + failure = _preference_failure( + _resolution_diagnostics, + merged_config.get("providers") or [], + provider_preferences, + ) + if failure is not None: + _spawn_preference_failure = { + "agent_name": agent_name, + "preferences_source": ( + "caller" if _caller_provider_preferences else "agent_overlay" + ), + **failure, + } # Apply orchestrator config override if specified (recipe-level rate limiting) # Session reads orchestrator config from: config["session"]["orchestrator"]["config"] @@ -1470,6 +1516,18 @@ async def child_resume_capability( # This gives us status, turn_count, and metadata from the orchestrator completion_data: dict = {} hooks = child_session.coordinator.get("hooks") + if _spawn_preference_failure is not None: + _spawn_preference_failure["session_id"] = sub_session_id + logger.warning( + "Sub-session %s: provider preference chain was unresolved " + "(reason=%s); selected provider=%s model=%s.", + sub_session_id, + _spawn_preference_failure["reason"], + _spawn_preference_failure["provider"], + _spawn_preference_failure["model"], + ) + if hooks: + await hooks.emit("provider:fallback", _spawn_preference_failure) unregister_hook = None if hooks: from amplifier_core.hooks import HookResult @@ -1535,6 +1593,8 @@ async def _capture_completion(event: str, data: dict) -> HookResult: # Store working_dir for session sync between CLI and web "working_dir": str(Path.cwd().resolve()), } + if _caller_provider_preferences: + metadata["caller_provider_preferences"] = _caller_provider_preferences # This persistence metadata is intentionally outside config/session # metadata, so it is never emitted through kernel telemetry. if base_prompt_snapshot: @@ -1650,6 +1710,19 @@ def _normalize_model_role(model_role: str | list[str] | None) -> list[str]: return [role for role in model_role if isinstance(role, str)] +def _serialize_provider_preferences(raw: Any) -> list[dict[str, Any]]: + """Return the durable, full ordered preference chain without mutating it.""" + if not raw: + return [] + serialized: list[dict[str, Any]] = [] + for entry in raw: + if isinstance(entry, dict): + serialized.append(copy.deepcopy(entry)) + elif callable(getattr(entry, "to_dict", None)): + serialized.append(copy.deepcopy(entry.to_dict())) + return serialized + + def _coerce_provider_preferences(raw: Any) -> list: """Coerce persisted/passed preferences to ProviderPreference objects. @@ -1664,19 +1737,26 @@ def _coerce_provider_preferences(raw: Any) -> list: from amplifier_foundation.spawn_utils import ProviderPreference coerced: list = [] - for entry in raw: + for index, entry in enumerate(raw): if isinstance(entry, ProviderPreference): coerced.append(entry) continue if isinstance(entry, dict): try: coerced.append(ProviderPreference.from_dict(entry)) - except ValueError as e: + except ValueError: logger.warning( - "Skipping malformed provider preference %r: %s", entry, e + "Skipping malformed provider preference at index %d " + "(type=%s; reason=validation_error).", + index, + type(entry).__name__, ) continue - logger.warning("Skipping unusable provider preference %r", entry) + logger.warning( + "Skipping unusable provider preference at index %d (type=%s).", + index, + type(entry).__name__, + ) return coerced @@ -1694,22 +1774,35 @@ def _provider_entry_keys(entry: dict) -> set[str]: 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. +def _legacy_resolution_is_verified(providers: list, preferences: list) -> bool: + """Return whether an older Foundation's outcome is independently verifiable. - 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. + Pre-diagnostics Foundation versions cannot distinguish a matching provider + from a failed catalog/auth lookup for a glob. Concrete preferences are the + sole exception: the mounted provider, model, and priority can be checked + directly without inferring why Foundation selected it. """ - wanted = {pref.provider for pref in preferences} + if not preferences or any( + not isinstance(preference.provider, str) + or not isinstance(preference.model, str) + or any(character in preference.model for character in "*?[") + for preference in preferences + ): + return False + for entry in providers or []: if not isinstance(entry, dict): continue - if (entry.get("config") or {}).get("priority") != 0: + config = entry.get("config") or {} + if config.get("priority") != 0: continue - if _provider_entry_keys(entry) & wanted: - return entry - return None + for preference in preferences: + if ( + preference.provider in _provider_entry_keys(entry) + and config.get("default_model") == preference.model + ): + return True + return False def _effective_provider(providers: list) -> dict | None: @@ -1734,6 +1827,194 @@ def _effective_provider(providers: list) -> dict | None: return best +async def _apply_provider_preferences( + config: dict, + preferences: list, + coordinator: Any, +) -> tuple[dict, list[Any] | None]: + """Call Foundation's resolver, opting into its diagnostics when available. + + CLI releases supported before Foundation's diagnostics sink remain usable. + The compatibility decision is based on the callable's inspected signature, + never by swallowing an unrelated ``TypeError`` raised inside Foundation. + + Release gate: retain this legacy branch until Foundation's diagnostics + contract has merged and the manager refreshes ``uv.lock``. Do not pin an + unmerged Foundation source in this package to bypass that release gate. + """ + from amplifier_foundation import apply_provider_preferences_with_resolution + + parameters = inspect.signature( + apply_provider_preferences_with_resolution + ).parameters.values() + supports_diagnostics = any( + parameter.name == "diagnostics" + or parameter.kind is inspect.Parameter.VAR_KEYWORD + for parameter in parameters + ) + diagnostics: list[Any] | None = [] if supports_diagnostics else None + if diagnostics is None: + return ( + await apply_provider_preferences_with_resolution( + config, preferences, coordinator + ), + None, + ) + return ( + await apply_provider_preferences_with_resolution( + config, preferences, coordinator, diagnostics=diagnostics + ), + diagnostics, + ) + + +def _preference_failure( + diagnostics: list[Any] | None, + providers: list, + preferences: list, +) -> dict[str, Any] | None: + """Describe an unresolved preference chain, or ``None`` after success.""" + if diagnostics is not None: + if any(getattr(result, "status", None) == "resolved" for result in diagnostics): + return None + terminal = diagnostics[-1] if diagnostics else None + statuses = [ + getattr(result, "status", None) for result in diagnostics if result is not None + ] + if statuses and all(status == "provider_not_mounted" for status in statuses): + reason = "preferred_provider_not_mounted" + elif getattr(terminal, "status", None): + reason = terminal.status + else: + reason = "provider_preferences_unresolved" + else: + # Older Foundation has no result diagnostics. Never infer a glob + # resolution from priority alone: it can hide an unavailable catalog or + # authentication failure. Exact preferences can be verified from the + # resulting provider/model/priority tuple; every other case gets one + # honest CLI diagnostic instead of a false success or absence claim. + if _legacy_resolution_is_verified(providers, preferences): + return None + # Absence from the plan is knowable without a model catalog. Only + # resolution for a present provider needs the legacy uncertainty label. + wanted = {pref.provider for pref in preferences} + present = any( + _provider_entry_keys(entry) & wanted + for entry in providers or [] + if isinstance(entry, dict) + ) + reason = ( + "legacy_resolution_unverified" + if present + else "preferred_provider_not_mounted" + ) + + landed = _effective_provider(providers) + return { + "reason": reason, + "provider": (landed or {}).get("module"), + "model": ((landed or {}).get("config") or {}).get("default_model"), + } + + +def _provider_override_configs(overrides: Any) -> dict[str, dict[str, Any]]: + """Index live provider overrides by both module and instance identity.""" + result: dict[str, dict[str, Any]] = {} + for override in overrides or []: + if not isinstance(override, dict) or not isinstance( + override.get("config"), dict + ): + continue + for key in (override.get("module"), override.get("id")): + if isinstance(key, str): + result[key] = override["config"] + return result + + +def _refresh_section_redacted_secrets( + section: Any, + config_overrides: dict[str, Any], + *, + provider_overrides: dict[str, dict[str, Any]] | None = None, + notification_overrides: dict[str, dict[str, Any]] | None = None, +) -> Any: + """Restore redacted secret leaves in one active or registered module list.""" + if not isinstance(section, list): + return section + + from amplifier_app_cli.runtime.config import restore_redacted_secret_values + + refreshed: list[Any] = [] + changed = False + for item in section: + if not isinstance(item, dict) or not isinstance(item.get("config"), dict): + refreshed.append(item) + continue + module = item.get("module") + identities = (item.get("id"), module) + live_configs: list[dict[str, Any]] = [] + if isinstance(module, str) and isinstance(config_overrides.get(module), dict): + live_configs.append(config_overrides[module]) + for mapping in (provider_overrides, notification_overrides): + if mapping: + for identity in identities: + if isinstance(identity, str) and isinstance(mapping.get(identity), dict): + live_configs.append(mapping[identity]) + + config = item["config"] + for live_config in live_configs: + config = restore_redacted_secret_values(config, live_config) + if config != item["config"]: + refreshed.append({**item, "config": config}) + changed = True + else: + refreshed.append(item) + return refreshed if changed else section + + +def _refresh_registered_agent_secrets( + agents: Any, + config_overrides: dict[str, Any], + provider_overrides: dict[str, dict[str, Any]], + notification_overrides: dict[str, dict[str, Any]], +) -> Any: + """Refresh registered agents recursively without adding absent modules.""" + if not isinstance(agents, dict): + return agents + result: dict[str, Any] = {} + changed = False + for name, agent_config in agents.items(): + if not isinstance(agent_config, dict): + result[name] = agent_config + continue + refreshed = agent_config + for section_name in ("providers", "tools", "hooks"): + section = refreshed.get(section_name) + new_section = _refresh_section_redacted_secrets( + section, + config_overrides, + provider_overrides=( + provider_overrides if section_name == "providers" else None + ), + notification_overrides=( + notification_overrides if section_name == "hooks" else None + ), + ) + if new_section is not section: + refreshed = {**refreshed, section_name: new_section} + nested = _refresh_registered_agent_secrets( + refreshed.get("agents"), + config_overrides, + provider_overrides, + notification_overrides, + ) + if nested is not refreshed.get("agents"): + refreshed = {**refreshed, "agents": nested} + result[name] = refreshed + changed = changed or refreshed is not agent_config + return result if changed else agents + + async def resume_sub_session( sub_session_id: str, instruction: str, @@ -1802,11 +2083,15 @@ async def resume_sub_session( ) from e # Extract reconstruction data - merged_config = metadata.get("config") - if not merged_config: + persisted_config = metadata.get("config") + if not persisted_config: raise RuntimeError( f"Corrupted session metadata for '{sub_session_id}'. Cannot reconstruct session without config." ) + # Never mutate the object SessionStore returned. Tests and callers may + # retain it, and resume-specific credential hydration belongs only to this + # reconstructed child. + merged_config = copy.deepcopy(persisted_config) parent_id = metadata.get("parent_id") agent_name = metadata.get("agent_name", "unknown") @@ -1839,106 +2124,60 @@ async def resume_sub_session( # overrides, then hook overrides, then env-var expansion) -- just applied # to the loaded snapshot instead of a freshly prepared bundle. # -------------------------------------------------------------------------- - if merged_config.get("providers") or merged_config.get("hooks"): + if any( + merged_config.get(section) + for section in ("providers", "tools", "hooks", "agents") + ): from amplifier_app_cli.lib.settings import AppSettings from amplifier_app_cli.runtime.config import ( - _apply_hook_overrides, - _apply_provider_overrides, - _map_id_to_instance_id, - deep_merge, expand_env_vars, - narrow_overrides_to_secrets, ) _live_settings = AppSettings() + _config_overrides_raw = _live_settings.get_config_overrides() + _config_overrides = ( + _config_overrides_raw + if isinstance(_config_overrides_raw, dict) + else {} + ) + _provider_overrides = _provider_override_configs( + _live_settings.get_provider_overrides() + ) + _notification_overrides = _provider_override_configs( + _live_settings.get_notification_hook_overrides() + ) - if merged_config.get("providers"): - # 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 - ) - _refreshed_providers = _map_id_to_instance_id(_refreshed_providers) - merged_config = {**merged_config, "providers": _refreshed_providers} - logger.debug( - "Refreshed credentials for %d provider(s) at resume time", - len(_refreshed_providers), - ) - - if merged_config.get("hooks"): - # Generalization of the provider refresh above. Re-derive hook - # config from the SAME two live sources resolve_bundle_config() - # uses to build a fresh session's hooks section: - # 1. "overrides..config" in settings.yaml -- applies to - # ANY module id, hooks included (AppSettings.get_config_overrides()). - # 2. Dedicated notification hook overrides - # (AppSettings.get_notification_hook_overrides()). - # 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: - _refreshed_hooks = [ - { - **hook, - "config": deep_merge( - hook.get("config", {}) or {}, - _config_overrides[hook["module"]], - ), - } - if isinstance(hook, dict) - and hook.get("module") in _config_overrides - else hook - for hook in _refreshed_hooks - ] - _notification_overrides = _live_settings.get_notification_hook_overrides() - if _notification_overrides: - _refreshed_hooks = _apply_hook_overrides( - _refreshed_hooks, _notification_overrides - ) - merged_config = {**merged_config, "hooks": _refreshed_hooks} - logger.debug( - "Refreshed credentials for %d hook(s) at resume time", - len(_refreshed_hooks), + _refreshed_config = dict(merged_config) + for _section_name in ("providers", "tools", "hooks"): + _section = merged_config.get(_section_name) + _new_section = _refresh_section_redacted_secrets( + _section, + _config_overrides, + provider_overrides=( + _provider_overrides + if _section_name == "providers" + else None + ), + notification_overrides=( + _notification_overrides if _section_name == "hooks" else None + ), ) + if _new_section is not _section: + _refreshed_config[_section_name] = _new_section + + _new_agents = _refresh_registered_agent_secrets( + merged_config.get("agents"), + _config_overrides, + _provider_overrides, + _notification_overrides, + ) + if _new_agents is not merged_config.get("agents"): + _refreshed_config["agents"] = _new_agents + merged_config = _refreshed_config - # Expand any ${VAR} references now that live overrides have been - # spliced in -- covers both providers and hooks in one pass. + # Live secret values may be ${ENV} placeholders. Expand after + # restoration, once, without importing any module absent from the + # persisted plan. merged_config = expand_env_vars(merged_config) # Fail-loud guard: if a secret-bearing field STILL reads the @@ -1955,22 +2194,27 @@ async def resume_sub_session( # Scan the ENTIRE merged config, not just hooks. The same silent- # sentinel failure mode exists wherever a secret can live: a provider # entry with no matching live override keeps its redacted key, tools - # are not re-hydrated on resume, and any of these can also appear + # are re-hydrated on resume, and any of these can also appear # agent-scoped under agents[*]. _find_redacted_values already recurses # arbitrary structures, so pointing it at the whole config closes the # gap at no extra cost. _redacted_paths = _find_redacted_values(merged_config) if _redacted_paths: + _active_paths = [ + path for path in _redacted_paths if not path.startswith(".agents.") + ] + _dormant_agent_paths = [ + path for path in _redacted_paths if path.startswith(".agents.") + ] logger.warning( - "Sub-session %s: %d config field(s) still hold the " - "redaction sentinel '%s' after credential refresh (no live " - "override found to restore them): %s. These fields are " - "mounted as-is; the destination/consumer is expected to " - "reject them rather than receive a fake credential.", + "Sub-session %s: credential refresh left %d active config " + "field(s) and %d dormant registered-agent field(s) redacted " + "(no matching usable live value): active=%s dormant_agents=%s.", sub_session_id, - len(_redacted_paths), - _REDACTION_SENTINEL, - _redacted_paths, + len(_active_paths), + len(_dormant_agent_paths), + _active_paths, + _dormant_agent_paths, ) # --- Rebuild the provider promotion -------------------------------------- @@ -1989,12 +2233,17 @@ async def resume_sub_session( "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. + # Precedence: current caller > persisted caller override > original agent + # overlay > legacy saved mount plan. ``caller_provider_preferences`` is + # intentionally separate from the agent overlay so one turn's caller + # routing never rewrites the agent definition used by future children. _resume_preferences = _coerce_provider_preferences(provider_preferences) _preferences_source = "caller" + if not _resume_preferences: + _resume_preferences = _coerce_provider_preferences( + metadata.get("caller_provider_preferences") + ) + _preferences_source = "persisted_caller" if not _resume_preferences: _resume_preferences = _coerce_provider_preferences( _resume_agent_overlay.get("provider_preferences") @@ -2006,58 +2255,67 @@ async def resume_sub_session( ) _preferences_source = "persisted_config" + _canonical_resume_preferences = _serialize_provider_preferences( + _resume_preferences + ) + if _canonical_resume_preferences: + merged_config = { + **merged_config, + "provider_preferences": _canonical_resume_preferences, + } + else: + merged_config = dict(merged_config) + merged_config.pop("provider_preferences", None) + + # An explicit resume override becomes the persisted caller override for + # the next cold resume. Do not replace it with an agent default merely + # because this leg happened to use that recovery source. + _explicit_resume_preferences = _serialize_provider_preferences( + provider_preferences + ) + if _explicit_resume_preferences: + metadata["caller_provider_preferences"] = _explicit_resume_preferences + _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. + # parent_session may be absent for a cold/root resume. Foundation's + # result diagnostics distinguish that a mounted provider's model + # catalog was unavailable from a provider that was absent entirely. _resume_coordinator = ( parent_session.coordinator if parent_session is not None else None ) - merged_config = await apply_provider_preferences_with_resolution( + merged_config, _resolution_diagnostics = await _apply_provider_preferences( merged_config, _resume_preferences, _resume_coordinator ) - _promoted = _find_promoted_provider( - merged_config.get("providers") or [], _resume_preferences + _failure = _preference_failure( + _resolution_diagnostics, + merged_config.get("providers") or [], + _resume_preferences, ) - if _promoted is not None: + if _failure is None: logger.debug( "Sub-session %s: re-applied provider promotion on resume " - "(provider=%s, model=%s, preferences from %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"), + "requested": [ + {"provider": preference.provider, "model": preference.model} + for preference in _resume_preferences + ], + **_failure, } - 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"], - ) + + # Persist the reconstructed mount plan, including a current explicit + # caller chain and any resolved promotion. Otherwise a later cold resume + # sees the previous config chain even though caller metadata was updated. + metadata["config"] = merged_config # Sub-session resume creates fresh UX systems. Parent UX context (approval history, # display state) is not preserved across resume. This is acceptable because: @@ -2281,6 +2539,15 @@ async def child_resume_capability( # the distinction the rc0 archive had no way to make. if _promotion_fallback: await hooks.emit("provider:fallback", _promotion_fallback) + if _promotion_fallback: + logger.warning( + "Sub-session %s: provider preference chain was unresolved " + "(reason=%s); selected provider=%s model=%s.", + sub_session_id, + _promotion_fallback["reason"], + _promotion_fallback["provider"], + _promotion_fallback["model"], + ) # Restore the resolved frozen snapshot first. Old named children did not # persist one, so retain their overlay/config reconstruction fallback. diff --git a/docs/SPAWN_PRECEDENCE.md b/docs/SPAWN_PRECEDENCE.md index 77db10f8..b992f1f7 100644 --- a/docs/SPAWN_PRECEDENCE.md +++ b/docs/SPAWN_PRECEDENCE.md @@ -69,6 +69,30 @@ The kernel doesn't enforce any precedence. The capability contract is just "spawn a sub-session" — what each implementation does with provider preferences is its own choice. +## Resume continuity + +Resume reconstructs a child from a redacted persisted mount plan. Before the +child is mounted it writes the one effective, serialized preference chain back +to `config.provider_preferences`. The sources are ordered: + +1. Preferences supplied to this resume call. +2. The prior explicit caller override saved as + `caller_provider_preferences`. +3. The persisted agent overlay's authored preferences. +4. Legacy `config.provider_preferences` from sessions saved before the + caller-provenance field existed. + +The separate caller field is deliberate: a caller's temporary routing choice +must survive a cold resume without mutating the agent definition that new +children will inherit. An explicit override on a later resume replaces that +field for subsequent cold resumes. + +Credential refresh is also resume-only. It restores only sensitive leaves that +are exactly `[REDACTED]` from matching live provider/module settings, including +registered agent module sections. It never deep-merges live settings into the +persisted plan, so child URLs, priority order, and routing configuration remain +the child's own. + ## In-process system-prompt inheritance For an in-process `agent_name: self` child, the CLI renders the root prepared diff --git a/tests/test_narrow_overrides_to_secrets.py b/tests/test_narrow_overrides_to_secrets.py index f9cc0a32..23f7fa4c 100644 --- a/tests/test_narrow_overrides_to_secrets.py +++ b/tests/test_narrow_overrides_to_secrets.py @@ -19,6 +19,7 @@ from amplifier_app_cli.runtime.config import _apply_provider_overrides from amplifier_app_cli.runtime.config import narrow_overrides_to_secrets +from amplifier_app_cli.runtime.config import restore_redacted_secret_values # --------------------------------------------------------------------------- @@ -241,3 +242,227 @@ def test_malformed_entries_are_skipped_not_raised(self): def test_empty_input_is_empty_output(self): assert narrow_overrides_to_secrets([]) == [] + + +def test_restore_only_replaces_a_redacted_matching_secret_leaf(): + """Live settings cannot rewrite a child's URL or unredacted secret.""" + persisted = { + "sources": { + "source-a": { + "url": "https://child-a.invalid", + "api_key": "[REDACTED]", + }, + "source-b": { + "url": "https://child-b.invalid", + "api_key": "child-b-kept", + }, + } + } + live = { + "sources": { + "source-a": { + "url": "https://settings-must-not-win.invalid", + "api_key": "live-a-key", + }, + "source-b": { + "url": "https://settings-must-not-win.invalid", + "api_key": "live-b-must-not-replace", + }, + } + } + + assert restore_redacted_secret_values(persisted, live) == { + "sources": { + "source-a": { + "url": "https://child-a.invalid", + "api_key": "live-a-key", + }, + "source-b": { + "url": "https://child-b.invalid", + "api_key": "child-b-kept", + }, + } + } + assert persisted["sources"]["source-a"]["api_key"] == "[REDACTED]" + + +def test_restore_keyed_dict_sources_remain_supported(): + persisted = { + "sources": { + "source-a": { + "url": "https://child-a.invalid", + "api_key": "[REDACTED]", + } + } + } + live = { + "sources": { + "source-a": { + "url": "https://live-must-not-replace.invalid", + "api_key": "fake-key-a", + } + } + } + + restored = restore_redacted_secret_values(persisted, live) + + assert restored == { + "sources": { + "source-a": { + "url": "https://child-a.invalid", + "api_key": "fake-key-a", + } + } + } + + +def test_restore_reordered_list_sources_matches_unique_ids(): + persisted = { + "sources": [ + {"id": "source-a", "url": "https://child-a.invalid", "api_key": "[REDACTED]"}, + {"id": "source-b", "url": "https://child-b.invalid", "api_key": "[REDACTED]"}, + ] + } + live = { + "sources": [ + {"id": "source-b", "url": "https://live-b.invalid", "api_key": "fake-key-b"}, + {"id": "source-a", "url": "https://live-a.invalid", "api_key": "fake-key-a"}, + ] + } + + restored = restore_redacted_secret_values(persisted, live) + + assert restored["sources"] == [ + {"id": "source-a", "url": "https://child-a.invalid", "api_key": "fake-key-a"}, + {"id": "source-b", "url": "https://child-b.invalid", "api_key": "fake-key-b"}, + ] + + +def test_restore_duplicate_or_absent_list_identity_keeps_redaction(): + duplicated = { + "sources": [ + {"id": "source-a", "url": "https://child-a.invalid", "api_key": "[REDACTED]"}, + {"id": "source-a", "url": "https://child-other.invalid", "api_key": "[REDACTED]"}, + ] + } + absent = { + "sources": [ + {"id": "source-a", "url": "https://child-a.invalid", "api_key": "[REDACTED]"} + ] + } + + duplicated_restored = restore_redacted_secret_values( + duplicated, + { + "sources": [ + {"id": "source-a", "url": "https://live-a.invalid", "api_key": "fake-key-a"} + ] + }, + ) + absent_restored = restore_redacted_secret_values( + absent, + { + "sources": [ + {"id": "source-b", "url": "https://child-a.invalid", "api_key": "fake-key-b"} + ] + }, + ) + + assert duplicated_restored == duplicated + assert absent_restored == absent + + +def test_restore_identityless_list_requires_unique_nonsecret_match(): + persisted = { + "sources": [ + {"url": "https://child-a.invalid", "api_key": "[REDACTED]"}, + {"url": "https://child-b.invalid", "api_key": "[REDACTED]"}, + ] + } + reordered_live = { + "sources": [ + {"url": "https://child-b.invalid", "api_key": "fake-key-b"}, + {"url": "https://child-a.invalid", "api_key": "fake-key-a"}, + ] + } + no_match_live = { + "sources": [ + {"url": "https://other.invalid", "api_key": "fake-key-b"}, + ] + } + + restored = restore_redacted_secret_values(persisted, reordered_live) + unmatched = restore_redacted_secret_values(persisted, no_match_live) + + assert [source["api_key"] for source in restored["sources"]] == [ + "fake-key-a", + "fake-key-b", + ] + assert unmatched == persisted + + +def test_restore_identityless_list_matches_nested_secret_structure(): + persisted = { + "sources": [ + { + "url": "https://child-a.invalid", + "connection": {"api_key": "[REDACTED]"}, + }, + { + "url": "https://child-b.invalid", + "connection": {"api_key": "[REDACTED]"}, + }, + ] + } + live = { + "sources": [ + { + "url": "https://child-b.invalid", + "connection": {"api_key": "fake-key-b"}, + }, + { + "url": "https://child-a.invalid", + "connection": {"api_key": "fake-key-a"}, + }, + ] + } + + restored = restore_redacted_secret_values(persisted, live) + + assert restored == { + "sources": [ + { + "url": "https://child-a.invalid", + "connection": {"api_key": "fake-key-a"}, + }, + { + "url": "https://child-b.invalid", + "connection": {"api_key": "fake-key-b"}, + }, + ] + } + + +def test_restore_identityless_list_keeps_nested_secrets_when_match_is_ambiguous(): + persisted = { + "sources": [ + { + "url": "https://shared.invalid", + "connection": {"api_key": "[REDACTED]"}, + } + ] + } + live = { + "sources": [ + { + "url": "https://shared.invalid", + "connection": {"api_key": "fake-key-a"}, + }, + { + "url": "https://shared.invalid", + "connection": {"api_key": "fake-key-b"}, + }, + ] + } + + assert restore_redacted_secret_values(persisted, live) == persisted diff --git a/tests/test_resume_preserves_provider_promotion.py b/tests/test_resume_preserves_provider_promotion.py index 3a9a53d7..ec26042c 100644 --- a/tests/test_resume_preserves_provider_promotion.py +++ b/tests/test_resume_preserves_provider_promotion.py @@ -47,13 +47,17 @@ from __future__ import annotations +import copy import logging +from types import SimpleNamespace 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_spawner import spawn_sub_session +from amplifier_app_cli.session_spawner import _coerce_provider_preferences from amplifier_app_cli.session_store import SessionStore pytestmark = pytest.mark.anyio @@ -199,6 +203,7 @@ async def _run_resume( session_id: str, *, provider_overrides: list[dict] | None = None, + config_overrides: dict[str, dict] | None = None, **resume_kwargs, ) -> tuple[dict, _RecordingHooks]: """Drive the real resume_sub_session() and capture the mounted config. @@ -236,7 +241,9 @@ def _capture(*args, **kwargs): 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_config_overrides = MagicMock( + return_value=config_overrides if config_overrides is not None else {} + ) mock_settings.get_notification_hook_overrides = MagicMock(return_value=[]) with ( @@ -414,8 +421,9 @@ async def test_model_role_is_written_into_the_resumed_config( assert config["model_role"] == ["fast"] + @pytest.mark.parametrize("legacy_resolver", [False, True]) async def test_unhonourable_promotion_emits_a_fallback_event( - self, tmp_path, monkeypatch, caplog + self, tmp_path, monkeypatch, caplog, legacy_resolver ): """Acceptance criterion: name the cause, do not silently re-resolve. @@ -435,11 +443,21 @@ async def test_unhonourable_promotion_emits_a_fallback_event( ) store.save(session_id, [], metadata) + if legacy_resolver: + async def legacy_apply(config, preferences, coordinator): + # No diagnostics API and no matching provider to promote. + return config + + monkeypatch.setattr( + "amplifier_foundation.apply_provider_preferences_with_resolution", + legacy_apply, + ) + 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, ( + assert len(fallbacks) == 1, ( "An unhonourable promotion on resume must emit a named fallback " "event rather than silently re-resolving by settings priority." ) @@ -470,3 +488,611 @@ async def test_no_preferences_leaves_the_plan_byte_identical( assert config["providers"] == _persisted_child_providers() assert not [n for n, _ in hooks.emitted if n == "provider:fallback"] + + async def test_persisted_caller_chain_wins_over_agent_default( + self, tmp_path, monkeypatch + ): + """Cold resume preserves the caller's whole fallback chain.""" + store = SessionStore() + session_id = "test-resume-persisted-caller-chain" + caller_chain = [ + { + "provider": "luna", + "model": "gpt-5.6-luna", + "config": {"reasoning_effort": "xhigh"}, + }, + {"provider": "sol", "model": "gpt-5.6-sol"}, + ] + metadata = _base_metadata( + session_id, + caller_provider_preferences=copy.deepcopy(caller_chain), + agent_overlay={ + "provider_preferences": [ + { + "provider": "luna", + "model": "gpt-5.6-luna", + "config": {"reasoning_effort": "medium"}, + } + ] + }, + ) + # This is intentionally stale legacy state. The resumed constructor + # must receive caller xhigh, not this medium value. + metadata["config"]["provider_preferences"] = metadata["agent_overlay"][ + "provider_preferences" + ] + store.save(session_id, [], metadata) + + config, _ = await _run_resume(session_id) + + assert config["provider_preferences"] == caller_chain + _, saved = store.load(session_id) + assert saved["caller_provider_preferences"] == caller_chain + assert saved["agent_overlay"]["provider_preferences"][0]["config"] == { + "reasoning_effort": "medium" + } + + async def test_explicit_resume_chain_survives_next_cold_resume( + self, tmp_path, monkeypatch + ): + """A later explicit override is not lost after this resumed turn.""" + store = SessionStore() + session_id = "test-resume-updated-caller-chain" + metadata = _base_metadata( + session_id, + caller_provider_preferences=[ + {"provider": "luna", "model": "gpt-5.6-luna"} + ], + agent_overlay={ + "provider_preferences": [ + {"provider": "sol", "model": "gpt-5.6-sol"} + ] + }, + ) + store.save(session_id, [], metadata) + updated_chain = [{"provider": "sol", "model": "gpt-5.6-sol"}] + + config, _ = await _run_resume( + session_id, provider_preferences=updated_chain + ) + assert config["provider_preferences"] == updated_chain + + cold_config, _ = await _run_resume(session_id) + assert cold_config["provider_preferences"] == updated_chain + + async def test_spawn_and_two_cold_resumes_preserve_caller_precedence( + self, tmp_path, monkeypatch + ): + """Exercise disk persistence without pre-seeding caller metadata or save().""" + session_id = "test-spawn-cold-resume-caller-precedence" + agent_chain = [ + { + "provider": "luna", + "model": "gpt-5.6-luna", + "config": {"reasoning_effort": "medium"}, + } + ] + caller_chain = [ + { + "provider": "luna", + "model": "gpt-5.6-luna", + "config": {"reasoning_effort": "xhigh"}, + }, + {"provider": "sol", "model": "gpt-5.6-sol"}, + ] + latest_caller_chain = [ + { + "provider": "sol", + "model": "gpt-5.6-sol", + "config": {"reasoning_effort": "high"}, + } + ] + agent_configs = { + "routing-agent": {"provider_preferences": copy.deepcopy(agent_chain)} + } + original_agent_configs = copy.deepcopy(agent_configs) + + parent_coordinator = MagicMock() + parent_coordinator.config = {} + parent_coordinator.get.return_value = None + parent_coordinator.get_capability.return_value = None + parent_coordinator.display_system = MagicMock() + parent_coordinator.approval_system = MagicMock() + parent_coordinator.cancellation = MagicMock() + parent_session = MagicMock() + parent_session.coordinator = parent_coordinator + parent_session.config = { + "session": {"orchestrator": "loop-basic", "context": "context-simple"}, + "providers": _persisted_child_providers(), + } + parent_session.session_id = "parent-cold-resume-test" + parent_session.trace_id = "trace-cold-resume-test" + parent_session.loader = None + + spawn_context = _FakeContext() + spawn_hooks = _RecordingHooks() + spawned_coordinator = MagicMock() + spawned_coordinator.get.side_effect = lambda name: { + "context": spawn_context, + "hooks": spawn_hooks, + }.get(name) + spawned_coordinator.get_capability.return_value = None + spawned_coordinator.mount = AsyncMock() + spawned_coordinator.collect_contributions = AsyncMock(return_value=[]) + spawned_coordinator.cancellation = MagicMock() + spawned_session = MagicMock() + spawned_session.coordinator = spawned_coordinator + spawned_session.initialize = AsyncMock() + spawned_session.execute = AsyncMock(return_value="response") + spawned_session.cleanup = AsyncMock() + spawned_session.session_id = session_id + + async def resolved(config, preferences, coordinator): + return config, [SimpleNamespace(status="resolved")] + + with ( + patch( + "amplifier_app_cli.session_spawner.AmplifierSession", + return_value=spawned_session, + ), + patch( + "amplifier_app_cli.session_spawner._apply_provider_preferences", + new=resolved, + ), + patch("amplifier_app_cli.paths.create_foundation_resolver"), + ): + await spawn_sub_session( + agent_name="routing-agent", + instruction="first turn", + parent_session=parent_session, + agent_configs=agent_configs, + sub_session_id=session_id, + provider_preferences=caller_chain, + ) + + _, spawned_metadata = SessionStore().load(session_id) + assert spawned_metadata["caller_provider_preferences"] == caller_chain + assert spawned_metadata["config"]["provider_preferences"] == caller_chain + assert spawned_metadata["agent_overlay"]["provider_preferences"] == agent_chain + + with patch( + "amplifier_app_cli.session_spawner._apply_provider_preferences", + new=resolved, + ): + first_cold_config, _ = await _run_resume(session_id) + assert first_cold_config["provider_preferences"] == caller_chain + + overridden_config, _ = await _run_resume( + session_id, provider_preferences=latest_caller_chain + ) + assert overridden_config["provider_preferences"] == latest_caller_chain + + second_cold_config, _ = await _run_resume(session_id) + + _, final_metadata = SessionStore().load(session_id) + assert second_cold_config["provider_preferences"] == latest_caller_chain + assert final_metadata["caller_provider_preferences"] == latest_caller_chain + assert final_metadata["config"]["provider_preferences"] == latest_caller_chain + assert final_metadata["agent_overlay"]["provider_preferences"] == agent_chain + assert agent_configs == original_agent_configs + + +class TestResumeNestedCredentialRefresh: + """Nested tools and registered agents get secret-only restoration.""" + + async def test_restores_nested_tool_credentials_without_rewriting_urls( + self, tmp_path, monkeypatch + ): + store = SessionStore() + session_id = "test-resume-nested-tool-secrets" + nested_tool = { + "module": "tool-graph-fixture", + "config": { + "sources": { + "source-a": { + "url": "https://persisted-a.invalid", + "api_key": "[REDACTED]", + }, + "source-b": { + "url": "https://persisted-b.invalid", + "api_key": "child-b-kept", + }, + } + }, + } + metadata = _base_metadata( + session_id, + config={ + "session": { + "orchestrator": "loop-basic", + "context": "context-simple", + }, + "tools": [copy.deepcopy(nested_tool)], + "agents": { + "graph-fixture": {"tools": [copy.deepcopy(nested_tool)]} + }, + }, + ) + store.save(session_id, [], metadata) + live_overrides = { + "tool-graph-fixture": { + "sources": { + "source-a": { + "url": "https://live-must-not-replace.invalid", + "api_key": "live-a-key", + }, + "source-b": { + "url": "https://live-must-not-replace.invalid", + "api_key": "live-b-must-not-replace", + }, + } + } + } + live_input = copy.deepcopy(live_overrides) + + config, _ = await _run_resume( + session_id, config_overrides=live_overrides + ) + + for tool in ( + config["tools"][0], + config["agents"]["graph-fixture"]["tools"][0], + ): + sources = tool["config"]["sources"] + assert sources["source-a"] == { + "url": "https://persisted-a.invalid", + "api_key": "live-a-key", + } + assert sources["source-b"] == { + "url": "https://persisted-b.invalid", + # SessionStore redacts every secret before this real resume + # path loads it, so the matching live leaf is restored too. + "api_key": "live-b-must-not-replace", + } + assert live_overrides == live_input + + async def test_absent_nested_override_reports_one_dormant_diagnostic( + self, tmp_path, monkeypatch, caplog + ): + store = SessionStore() + session_id = "test-resume-missing-nested-tool-secret" + metadata = _base_metadata( + session_id, + config={ + "session": { + "orchestrator": "loop-basic", + "context": "context-simple", + }, + "agents": { + "offline-fixture": { + "tools": [ + { + "module": "tool-not-configured", + "config": {"api_key": "[REDACTED]"}, + } + ] + } + }, + }, + ) + store.save(session_id, [], metadata) + + with caplog.at_level(logging.WARNING): + await _run_resume(session_id, config_overrides={}) + + refresh_warnings = [ + record.message + for record in caplog.records + if "credential refresh left" in record.message + ] + assert len(refresh_warnings) == 1 + assert "0 active config field(s) and 1 dormant registered-agent" in ( + refresh_warnings[0] + ) + + async def test_unmatched_list_reports_one_aggregate_diagnostic( + self, tmp_path, monkeypatch, caplog + ): + store = SessionStore() + session_id = "test-resume-unmatched-tool-source-list" + secret_value = "must-not-appear-in-logs" + metadata = _base_metadata( + session_id, + agent_overlay={"instruction": "fixture prompt"}, + config={ + "session": { + "orchestrator": "loop-basic", + "context": "context-simple", + }, + "tools": [ + { + "module": "tool-source-fixture", + "config": { + "sources": [ + { + "url": "https://persisted.invalid", + "api_key": "[REDACTED]", + } + ] + }, + } + ], + }, + ) + store.save(session_id, [], metadata) + + with caplog.at_level(logging.WARNING): + config, _ = await _run_resume( + session_id, + config_overrides={ + "tool-source-fixture": { + "sources": [ + { + "url": "https://live.invalid", + "api_key": secret_value, + } + ] + } + }, + ) + + warnings = [ + record for record in caplog.records if record.levelno == logging.WARNING + ] + assert len(warnings) == 1 + assert "credential refresh left 1 active config field(s)" in warnings[0].message + assert secret_value not in warnings[0].message + assert config["tools"][0]["config"]["sources"][0]["api_key"] == "[REDACTED]" + + +class TestResumeResolutionDiagnostics: + """The CLI reports Foundation's final resolution outcome once.""" + + async def test_cold_glob_matching_persisted_model_consumes_resolution_diagnostics_quietly( + self, tmp_path, monkeypatch, caplog + ): + """The CLI consumes a mocked resolved diagnostics record without a fallback.""" + store = SessionStore() + session_id = "test-cold-anthropic-glob-matches-persisted-sonnet" + metadata = _base_metadata( + session_id, + config={ + "session": { + "orchestrator": "loop-basic", + "context": "context-simple", + }, + "providers": [ + { + "module": "provider-anthropic", + "config": { + "priority": 0, + "default_model": "claude-sonnet-4-6", + }, + } + ], + }, + agent_overlay={ + "provider_preferences": [ + {"provider": "anthropic", "model": "claude-sonnet-*"} + ] + }, + ) + store.save(session_id, [], metadata) + + async def resolved(config, preferences, coordinator, *, diagnostics=None): + assert diagnostics == [] + diagnostics.append( + SimpleNamespace( + status="resolved", + provider="anthropic", + model="claude-sonnet-4-6", + ) + ) + return config + + with ( + patch( + "amplifier_foundation.apply_provider_preferences_with_resolution", + new=resolved, + ), + caplog.at_level(logging.WARNING), + ): + config, hooks = await _run_resume(session_id) + + assert config["providers"][0]["config"]["default_model"] == "claude-sonnet-4-6" + assert not [ + event for event, _ in hooks.emitted if event == "provider:fallback" + ] + assert not [ + record + for record in caplog.records + if "provider preference chain was unresolved" in record.message + ] + + async def test_terminal_success_suppresses_fallback_diagnostic( + self, tmp_path, monkeypatch, caplog + ): + store = SessionStore() + session_id = "test-resume-terminal-resolution-success" + metadata = _base_metadata( + session_id, + agent_overlay={ + "provider_preferences": [ + {"provider": "luna", "model": "gpt-5.6-luna"} + ] + }, + ) + store.save(session_id, [], metadata) + + async def resolved(config, preferences, coordinator, *, diagnostics=None): + assert diagnostics is not None + diagnostics.append( + SimpleNamespace(status="resolved", provider="luna") + ) + return config + + with ( + patch( + "amplifier_foundation.apply_provider_preferences_with_resolution", + new=resolved, + ), + caplog.at_level(logging.WARNING), + ): + _, hooks = await _run_resume(session_id) + + assert not [event for event, _ in hooks.emitted if event == "provider:fallback"] + assert not [ + record + for record in caplog.records + if "provider preference chain was unresolved" in record.message + ] + + async def test_catalog_failure_emits_one_truthful_fallback( + self, tmp_path, monkeypatch, caplog + ): + store = SessionStore() + session_id = "test-resume-catalog-failure" + metadata = _base_metadata( + session_id, + config={ + "session": { + "orchestrator": "loop-basic", + "context": "context-simple", + }, + "providers": [ + { + "module": "provider-luna", + "config": { + "priority": 0, + "default_model": "gpt-5.6-luna", + }, + } + ], + }, + agent_overlay={ + "provider_preferences": [ + {"provider": "luna", "model": "gpt-5.6-*"} + ] + }, + ) + store.save(session_id, [], metadata) + + async def catalog_failed(config, preferences, coordinator, *, diagnostics=None): + assert diagnostics is not None + diagnostics.append( + SimpleNamespace(status="catalog_query_failed", provider="luna") + ) + return config + + with ( + patch( + "amplifier_foundation.apply_provider_preferences_with_resolution", + new=catalog_failed, + ), + caplog.at_level(logging.WARNING), + ): + _, hooks = await _run_resume(session_id) + + fallbacks = [data for event, data in hooks.emitted if event == "provider:fallback"] + assert fallbacks == [ + { + "session_id": session_id, + "agent_name": "git-ops", + "preferences_source": "agent_overlay", + "requested": [{"provider": "luna", "model": "gpt-5.6-*"}], + "reason": "catalog_query_failed", + "provider": "provider-luna", + "model": "gpt-5.6-luna", + } + ] + preference_warnings = [ + record + for record in caplog.records + if "provider preference chain was unresolved" in record.message + ] + assert len(preference_warnings) == 1 + + async def test_legacy_glob_resolution_is_unverified_without_hiding_failure( + self, tmp_path, monkeypatch, caplog + ): + """Older Foundation has no outcome contract, so its glob result is not guessed.""" + store = SessionStore() + session_id = "test-legacy-glob-resolution-unverified" + metadata = _base_metadata( + session_id, + agent_overlay={ + "provider_preferences": [ + {"provider": "luna", "model": "gpt-5.6-*"}, + ] + }, + ) + store.save(session_id, [], metadata) + + async def legacy_catalog_failure(config, preferences, coordinator): + logging.getLogger("foundation-test").warning( + "catalog authentication failure remains visible" + ) + return config + + with ( + patch( + "amplifier_foundation.apply_provider_preferences_with_resolution", + new=legacy_catalog_failure, + ), + caplog.at_level(logging.WARNING), + ): + _, hooks = await _run_resume(session_id) + + fallbacks = [data for event, data in hooks.emitted if event == "provider:fallback"] + assert fallbacks[0]["reason"] == "legacy_resolution_unverified" + assert "catalog authentication failure remains visible" in caplog.text + + async def test_legacy_concrete_resolution_is_verified_from_its_outcome( + self, tmp_path, monkeypatch, caplog + ): + """A legacy exact model match can be checked without guessing a catalog result.""" + store = SessionStore() + session_id = "test-legacy-concrete-resolution-verified" + metadata = _base_metadata( + session_id, + agent_overlay={ + "provider_preferences": [ + {"provider": "luna", "model": "gpt-5.6-luna"}, + ] + }, + ) + store.save(session_id, [], metadata) + + async def legacy_resolved(config, preferences, coordinator): + return config + + with ( + patch( + "amplifier_foundation.apply_provider_preferences_with_resolution", + new=legacy_resolved, + ), + caplog.at_level(logging.WARNING), + ): + _, hooks = await _run_resume(session_id) + + assert not [event for event, _ in hooks.emitted if event == "provider:fallback"] + assert "provider preference chain was unresolved" not in caplog.text + + +def test_malformed_provider_preferences_do_not_log_raw_configuration(caplog): + secret = "fake-provider-preference-secret" + + with ( + patch( + "amplifier_foundation.spawn_utils.ProviderPreference.from_dict", + side_effect=ValueError(f"invalid preference carrying {secret}"), + ), + caplog.at_level(logging.WARNING), + ): + assert _coerce_provider_preferences( + [{"provider": "luna", "config": {"api_key": secret}}, secret] + ) == [] + + assert secret not in caplog.text + assert "index 0 (type=dict; reason=validation_error)" in caplog.text + assert "index 1 (type=str)" in caplog.text diff --git a/tests/test_resume_redaction_guard.py b/tests/test_resume_redaction_guard.py index 136bbaee..e5209b99 100644 --- a/tests/test_resume_redaction_guard.py +++ b/tests/test_resume_redaction_guard.py @@ -7,8 +7,8 @@ mounted (which would surface downstream as a misleading 401). The guard scans the ENTIRE merged config, not just hooks: a provider entry -with no matching live override keeps its redacted key, tools are not -re-hydrated on resume, and any of these can also appear agent-scoped under +with no matching live override keeps its redacted key, tools are re-hydrated +on resume, and any of these can also appear agent-scoped under agents[*]. These tests pin that whole-config coverage. """ @@ -59,7 +59,7 @@ def test_redacted_hook_is_detected(): def test_redacted_tool_is_detected(): - """Tools are not re-hydrated on resume; the guard must still surface them.""" + """A tool with no usable live match remains visible to the guard.""" config = { "tools": [ {"module": "tool-remote", "config": {"token": _REDACTION_SENTINEL}}, diff --git a/tests/test_session_spawner.py b/tests/test_session_spawner.py index 92ce51e7..4daa5212 100644 --- a/tests/test_session_spawner.py +++ b/tests/test_session_spawner.py @@ -1194,7 +1194,7 @@ async def test_uses_agent_config_prefs_when_caller_passes_none( apply_called_with = {} - async def fake_apply_prefs(config, prefs, coordinator): + async def fake_apply_prefs(config, prefs, coordinator, *, diagnostics=None): apply_called_with["prefs"] = prefs return config # return unchanged for simplicity @@ -1312,7 +1312,7 @@ async def test_explicit_prefs_take_precedence_over_agent_config_prefs( apply_called_with = {} - async def fake_apply_prefs(config, prefs, coordinator): + async def fake_apply_prefs(config, prefs, coordinator, *, diagnostics=None): apply_called_with["prefs"] = prefs return config @@ -1378,11 +1378,18 @@ def child_get(name): } # Explicit caller pref — should win over agent_config pref - explicit_pref = ProviderPreference(provider="openai", model="gpt-5") + explicit_pref = ProviderPreference( + provider="openai", model="gpt-5", config={"reasoning_effort": "xhigh"} + ) + constructed_config = {} + + def capture_child_session(**kwargs): + constructed_config.update(kwargs["config"]) + return child_session with patch( "amplifier_app_cli.session_spawner.AmplifierSession", - return_value=child_session, + side_effect=capture_child_session, ): with patch( "amplifier_app_cli.session_spawner.generate_sub_session_id", @@ -1410,6 +1417,16 @@ def child_get(name): "Explicit caller prefs must override agent_config routing prefs (Bug A precedence)" ) assert applied[0].model == "gpt-5" + assert constructed_config["provider_preferences"] == [ + { + "provider": "openai", + "model": "gpt-5", + "config": {"reasoning_effort": "xhigh"}, + } + ] + assert agent_configs["coder"]["provider_preferences"] == [ + {"provider": "anthropic", "model": "claude-sonnet-4-6"} + ] async def test_no_prefs_no_agent_config_prefs_skips_apply( self, tmp_path, monkeypatch @@ -1422,7 +1439,7 @@ async def test_no_prefs_no_agent_config_prefs_skips_apply( apply_call_count = {"n": 0} - async def fake_apply_prefs(config, prefs, coordinator): + async def fake_apply_prefs(config, prefs, coordinator, *, diagnostics=None): apply_call_count["n"] += 1 return config diff --git a/uv.lock b/uv.lock index dfad2389..b721203b 100644 --- a/uv.lock +++ b/uv.lock @@ -49,7 +49,7 @@ dev = [ [[package]] name = "amplifier-core" -version = "1.6.0" +version = "1.6.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "click" }, @@ -59,18 +59,18 @@ dependencies = [ { name = "typing-extensions" }, ] wheels = [ - { url = "https://files.pythonhosted.org/packages/32/90/d520390cd91aae3d02db53653f828046089c79203dbb142e9bda346fa1d6/amplifier_core-1.6.0-cp311-abi3-macosx_10_12_x86_64.whl", hash = "sha256:d35130e4262cf0db2d6c5f7e65e244a9ef2c7397bfe2a9853bc9b0d9fd05be64", size = 8113151, upload-time = "2026-05-18T16:13:46.825Z" }, - { url = "https://files.pythonhosted.org/packages/94/75/3ab3126ba5a6f2fc6051a4d08e42364899e4c9ac4daa9d0a60947bf8acd1/amplifier_core-1.6.0-cp311-abi3-macosx_11_0_arm64.whl", hash = "sha256:387a2c58fcf4caefdb45c52ec228307bc225e73606897f242154782bc3e123da", size = 7268223, upload-time = "2026-05-18T16:13:48.749Z" }, - { url = "https://files.pythonhosted.org/packages/21/22/5a36160b3487170bcba0cbc61535101ff624e8314ed38fd35e561cb711a1/amplifier_core-1.6.0-cp311-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:8344fccdedd725a51c018de17867cdf1c35abb571dabc0bbccdb5c1242324a47", size = 7532259, upload-time = "2026-05-18T16:13:50.614Z" }, - { url = "https://files.pythonhosted.org/packages/bd/d7/3874c2308523209411367cf3b8b690e14e869f5f6bfb64cb1b1971e06a96/amplifier_core-1.6.0-cp311-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:5a8e0103242a2e2a975c880b1de0e5a02501e0421c1e5386dadae3f111e1d2b5", size = 8507642, upload-time = "2026-05-18T16:13:52.977Z" }, - { url = "https://files.pythonhosted.org/packages/86/59/3646a89537b4556274183519f6db9c354fb3d183f52ef4a2179af12dd386/amplifier_core-1.6.0-cp311-abi3-win_amd64.whl", hash = "sha256:5113aa2d88038776eb257af9e7d9de7af13b3cd9097d2ac67aef5730fa0678e3", size = 8910313, upload-time = "2026-05-18T16:13:55.249Z" }, - { url = "https://files.pythonhosted.org/packages/9f/9e/58b141115e5eea65703f0b01459eefed36b561e9642ba96d48542345cd8f/amplifier_core-1.6.0-cp311-abi3-win_arm64.whl", hash = "sha256:e1b2731dc09d1cbc668b411007e7f9a2c7edbd75b2525407cae1e6b4a4de0b83", size = 7661416, upload-time = "2026-05-18T16:13:57.513Z" }, + { url = "https://files.pythonhosted.org/packages/03/cd/8b0b520bf0de741ea73e069aaf64aca28c9f4ce91a7b8b9239193a6c4c1b/amplifier_core-1.6.1-cp311-abi3-macosx_10_12_x86_64.whl", hash = "sha256:c0f711d8408de78e53e5deddcb38b7240c5c1c497ca51eeaaeff23559b3d3c48", size = 8281633, upload-time = "2026-08-10T02:38:11.98Z" }, + { url = "https://files.pythonhosted.org/packages/14/83/f4fb297d87d35b9d74058da02bb153e12f7891ab62b3aaf7e0857f877798/amplifier_core-1.6.1-cp311-abi3-macosx_11_0_arm64.whl", hash = "sha256:b08f37e2c0b1611349a0e25d5bf9bfdfae3afcee35488f8e26bba1cdd400503b", size = 7366930, upload-time = "2026-08-10T02:38:14.105Z" }, + { url = "https://files.pythonhosted.org/packages/ff/ba/5eb9cecf92d8053c5e6d46ad9668c3ed3558d5423845c1dced1f266b2a38/amplifier_core-1.6.1-cp311-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:6ebf7e3993c76ea506e70ac7844b286c3ba2e9127b3bcb350fa4fcd2dcdbd38d", size = 7659512, upload-time = "2026-08-10T02:38:16.314Z" }, + { url = "https://files.pythonhosted.org/packages/22/31/121f054e3d079dc33d83f3d8ba9af50fd9f7694c3e2ba3d7d23d7c157d48/amplifier_core-1.6.1-cp311-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:3c957cd0671d2a003f2c8f7d6a41bd6e808f97d183c57b97e7700bf4c912621d", size = 8678425, upload-time = "2026-08-10T02:38:18.243Z" }, + { url = "https://files.pythonhosted.org/packages/35/25/bfc217f4a9ed2d033995fc59847f1fee2e1b17130632fcb0e0981a1a311b/amplifier_core-1.6.1-cp311-abi3-win_amd64.whl", hash = "sha256:50c80bcfa1f6efe769b19e7af18c925024c7553d4db08880727241709dd44eae", size = 8976601, upload-time = "2026-08-10T02:38:20.505Z" }, + { url = "https://files.pythonhosted.org/packages/a5/14/5f330452c92c6c5d35c51ad5311301949ce5db4d1a1a901456f3ee43eaac/amplifier_core-1.6.1-cp311-abi3-win_arm64.whl", hash = "sha256:cd8b617f132cf5d1ca3e5187d5f831d1f2a508bb40d07b2ab1085961bcb9e1a9", size = 7744837, upload-time = "2026-08-10T02:38:22.562Z" }, ] [[package]] name = "amplifier-foundation" version = "1.0.0" -source = { git = "https://github.com/microsoft/amplifier-foundation?branch=main#4f7c482438e05bd678eb92e899ef181a8a0e267e" } +source = { git = "https://github.com/microsoft/amplifier-foundation?branch=main#e210edabd947af82d5121a240d6934283ac540b9" } dependencies = [ { name = "amplifier-core" }, { name = "pyyaml" },