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_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 c7507fc8..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) @@ -1207,12 +1345,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 +1392,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: @@ -1302,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 @@ -1460,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( @@ -1503,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/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. 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([]) == [] 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"]