diff --git a/amplifier_app_cli/commands/session.py b/amplifier_app_cli/commands/session.py index 7c82579c..b4075f2a 100644 --- a/amplifier_app_cli/commands/session.py +++ b/amplifier_app_cli/commands/session.py @@ -108,7 +108,8 @@ def _prepare_resume_context( store = SessionStore() transcript, metadata = store.load(session_id) - # Extract bundle from saved session metadata + # SessionStore normalizes malformed parseable metadata at the shared read + # boundary; extract_session_mode also defensively ignores unusable bundles. saved_bundle, _ = extract_session_mode(metadata) bundle_name = None diff --git a/amplifier_app_cli/effective_config.py b/amplifier_app_cli/effective_config.py index b9737978..5f465557 100644 --- a/amplifier_app_cli/effective_config.py +++ b/amplifier_app_cli/effective_config.py @@ -39,6 +39,50 @@ def format_banner_line(self) -> str: return f"Bundle: {bundle_name} | Provider: {self.provider_name} | {self.model}" +@dataclass(frozen=True) +class EffectiveProviderModel: + """Canonical provider/model provenance for a resolved session config.""" + + provider: str + model: str + + def as_metadata(self) -> dict[str, str]: + """Return the fields persisted by every durable session writer.""" + return {"provider": self.provider, "model": self.model} + + +def get_effective_provider_model(config: dict[str, Any]) -> EffectiveProviderModel: + """Resolve the provider/model pair that will handle the session. + + Provider selection follows orchestrator priority semantics. Within the + selected provider's config, ``model`` is the supported explicit runtime + setting and therefore takes precedence over the legacy/default + ``default_model`` setting. + """ + providers = config.get("providers", []) + selected_provider = ( + _select_provider_by_priority(providers) if isinstance(providers, list) else None + ) + if selected_provider is None: + return EffectiveProviderModel(provider="none", model="none") + + provider = selected_provider.get("module") + if not isinstance(provider, str) or not provider: + provider = "unknown" + + provider_config = selected_provider.get("config", {}) + if not isinstance(provider_config, dict): + provider_config = {} + + model = provider_config.get("model") + if not isinstance(model, str) or not model: + model = provider_config.get("default_model") + if not isinstance(model, str) or not model: + model = "default" + + return EffectiveProviderModel(provider=provider, model=model) + + def get_effective_config_summary( config: dict[str, Any], config_source: str = "default", @@ -52,22 +96,14 @@ def get_effective_config_summary( Returns: EffectiveConfigSummary with display-friendly information """ - # Extract provider info - select by priority (lowest number wins) - # This matches the orchestrator's _select_provider() logic - providers = config.get("providers", []) - selected_provider = _select_provider_by_priority(providers) - - if selected_provider: - provider_module = selected_provider.get("module", "unknown") - provider_config = selected_provider.get("config", {}) - model = provider_config.get("default_model", "default") - + provenance = get_effective_provider_model(config) + provider_module = provenance.provider + model = provenance.model + if provider_module != "none": # Try to get friendly provider name provider_name = _get_provider_display_name(provider_module) else: - provider_module = "none" provider_name = "None" - model = "none" # Extract orchestrator session_config = config.get("session", {}) @@ -158,4 +194,9 @@ def _get_provider_display_name(provider_module: str) -> str: return name_map.get(name, name.replace("-", " ").title()) -__all__ = ["EffectiveConfigSummary", "get_effective_config_summary"] +__all__ = [ + "EffectiveConfigSummary", + "EffectiveProviderModel", + "get_effective_config_summary", + "get_effective_provider_model", +] diff --git a/amplifier_app_cli/incremental_save.py b/amplifier_app_cli/incremental_save.py index 2e327854..e65d527d 100644 --- a/amplifier_app_cli/incremental_save.py +++ b/amplifier_app_cli/incremental_save.py @@ -18,6 +18,7 @@ if TYPE_CHECKING: from amplifier_core import AmplifierSession +from .effective_config import get_effective_provider_model from .session_store import SessionStore logger = logging.getLogger(__name__) @@ -95,8 +96,7 @@ async def on_tool_post(self, event: str, data: dict[str, Any]): # Update debounce counter self._last_message_count = current_count - # Extract model name from config - model_name = self._extract_model_name() + provenance = get_effective_provider_model(self.config) # Load existing metadata to preserve fields like name, description # that may have been set by other hooks (e.g., session-naming) @@ -110,7 +110,7 @@ async def on_tool_post(self, event: str, data: dict[str, Any]): "created", datetime.now(UTC).isoformat() ), "bundle": self.bundle_name, - "model": model_name, + **provenance.as_metadata(), "turn_count": len([m for m in messages if m.get("role") == "user"]), "incremental": True, # Distinguish from final saves # Store working_dir for session sync between CLI and web @@ -131,22 +131,6 @@ async def on_tool_post(self, event: str, data: dict[str, Any]): return HookResult(action="continue") - def _extract_model_name(self) -> str: - """Extract model name from session config. - - Returns: - Model name string or "unknown" if not found - """ - providers = self.config.get("providers", []) - if isinstance(providers, list) and providers: - first_provider = providers[0] - if isinstance(first_provider, dict) and "config" in first_provider: - provider_config = first_provider["config"] - return provider_config.get("model") or provider_config.get( - "default_model", "unknown" - ) - return "unknown" - def register_incremental_save( session: "AmplifierSession", diff --git a/amplifier_app_cli/main.py b/amplifier_app_cli/main.py index de93a2ed..fa707faf 100644 --- a/amplifier_app_cli/main.py +++ b/amplifier_app_cli/main.py @@ -47,7 +47,10 @@ from .commands.version import version as version_cmd from .console import Markdown, console from .dedicated_tty_input import close_dedicated_tty_input, get_dedicated_tty_input -from .effective_config import get_effective_config_summary +from .effective_config import ( + get_effective_config_summary, + get_effective_provider_model, +) from .key_manager import KeyManager from .session_runner import SessionConfig, create_initialized_session from .session_store import SessionStore @@ -2881,17 +2884,6 @@ async def interactive_chat( ) ) - # Helper to extract model name from config - def _extract_model_name() -> str: - if isinstance(config.get("providers"), list) and config["providers"]: - first_provider = config["providers"][0] - if isinstance(first_provider, dict) and "config" in first_provider: - provider_config = first_provider["config"] - return provider_config.get("model") or provider_config.get( - "default_model", "unknown" - ) - return "unknown" - # Helper to save session after each turn async def _save_session(): context = session.coordinator.get("context") @@ -2900,6 +2892,7 @@ async def _save_session(): # Load existing metadata to preserve fields like name, description # that may have been set by other hooks (e.g., session-naming) existing_metadata = store.get_metadata(actual_session_id) or {} + provenance = get_effective_provider_model(config) metadata = { **existing_metadata, # Preserve name, description, etc. "session_id": actual_session_id, @@ -2907,7 +2900,7 @@ async def _save_session(): "created", datetime.now(UTC).isoformat() ), "bundle": bundle_name, - "model": _extract_model_name(), + **provenance.as_metadata(), "turn_count": len([m for m in messages if m.get("role") == "user"]), # Store working_dir for session sync between CLI and web "working_dir": str(Path.cwd().resolve()), @@ -3647,6 +3640,7 @@ def _goal_sigint_handler(signum, frame): # Load existing metadata to preserve fields like name, description # that may have been set by other hooks (e.g., session-naming) existing_metadata = store.get_metadata(actual_session_id) or {} + provenance = get_effective_provider_model(config) metadata = { **existing_metadata, # Preserve name, description, etc. "session_id": actual_session_id, @@ -3654,7 +3648,7 @@ def _goal_sigint_handler(signum, frame): "created", datetime.now(UTC).isoformat() ), "bundle": bundle_name, - "model": model_name, + **provenance.as_metadata(), "turn_count": len([m for m in messages if m.get("role") == "user"]), # Store working_dir for session sync between CLI and web "working_dir": str(Path.cwd().resolve()), diff --git a/amplifier_app_cli/session_runner.py b/amplifier_app_cli/session_runner.py index 16cb6d2b..288964ac 100644 --- a/amplifier_app_cli/session_runner.py +++ b/amplifier_app_cli/session_runner.py @@ -15,6 +15,7 @@ 6. Register session spawning capability 7. Restore transcript (resume only) 7.5. Restore cumulative session cost (resume only) +7.6. Warn (and confirm) on provider/model mismatch at resume (resume only) 8. Register approval provider Philosophy: @@ -25,19 +26,24 @@ from __future__ import annotations +import asyncio import logging import sys import uuid from collections import Counter from dataclasses import dataclass from dataclasses import field +from datetime import UTC +from datetime import datetime from pathlib import Path from typing import TYPE_CHECKING from typing import Any +import click from amplifier_core import AmplifierSession from amplifier_core import ModuleValidationError +from .effective_config import get_effective_provider_model from .lib.settings import AppSettings from .session_store import SessionStore from .ui.error_display import display_validation_error @@ -125,6 +131,7 @@ async def create_initialized_session( 5. Register mention handling capability 6. Register session spawning capability 7. Restore transcript (resume only) + 7.6. Warn (and confirm) on provider/model mismatch (resume only) 8. Register approval provider Args: @@ -290,6 +297,15 @@ async def create_initialized_session( except Exception: logger.debug("Prior session cost restore skipped", exc_info=True) + # Step 7.6: Warn (and, on a tty, confirm) on provider/model mismatch at + # resume - amplifier-support#208 Wave 2 / Option A. This is the ONE + # chokepoint every resume surface (amplifier run --resume, amplifier + # session resume, amplifier resume, amplifier continue) passes through, + # before the first session.execute() call. Runs only for resumes; silent + # (no console output, no prompt) when there's nothing to warn about. + if config.is_resume: + await _warn_on_resume_provider_mismatch(config, session_id, console, session) + # Step 10: Register approval provider (app-layer policy) from .approval_provider import CLIApprovalProvider from .stdin_arbiter import StdinArbiter @@ -333,6 +349,225 @@ async def create_initialized_session( ) +# ============================================================================= +# Resume-time provider/model mismatch check (amplifier-support#208, Wave 2) +# ============================================================================= +# +# Sessions that switch providers mid-conversation can persist provider-specific +# content (e.g. Anthropic thinking-block signatures) that bricks the session +# when replayed against a different provider (400s on invalid signatures). +# Wave 1 sanitized/repaired that content at the provider boundary. This is +# the resume-time guardrail (Option A, ack'd by the maintainer): warn -- and, +# interactively, require confirmation -- when the provider/model about to +# handle a resumed session differs from whichever provider/model last wrote +# its metadata. + + +def _normalize_provider_identity(value: object) -> str: + """Normalize a provider identity for mismatch comparison. + + Provider identity may be recorded as a module id ("provider-anthropic") + or a bare provider name ("anthropic") depending on the source/vintage of + the metadata. Strip the "provider-" prefix and lowercase so both forms + compare equal, regardless of which side (stored metadata vs. active + config) uses which form. + + Mirrors the nested _normalize_to_provider_name() helper inside + _should_attempt_self_healing() above, but is exposed at module level + since it is also needed here to compare against the "provider" field + persisted by main.py's session-save code. + + Args: + value: Provider identity string (module id or bare name), or None. + + Returns: + Lowercased bare provider name (e.g. "anthropic"), or "" if value is + falsy. + """ + if not isinstance(value, str) or not value: + return "" + return value.strip().lower().removeprefix("provider-") + + +async def _warn_on_resume_provider_mismatch( + config: SessionConfig, + session_id: str, + console: "Console", + session: AmplifierSession, +) -> None: + """Warn (and, on a tty, require confirmation) on provider/model mismatch. + + Compares the provider/model that is about to handle this resumed session + (the ACTIVE config, resolved with any --provider/--model overrides already + applied) against whichever provider/model last wrote this session's + metadata.json (the PRIOR writer). + + Feature-detection: pre-existing sessions (saved before this PR) have no + "provider" field in their metadata. For those, comparison falls back to + model-only. Sessions saved by this PR (or later) carry "provider" and get + the full provider+model comparison. + + On match, or when there is no prior metadata to compare against at all: + completely silent -- zero behavior change and zero console output. + + On mismatch: + - stdin is a tty: print the warning, then require explicit confirmation + via `click.confirm` (offloaded to a worker thread -- see the + KeyboardInterrupt handler in main.py for why this offload is mandatory: + click.confirm() performs a blocking, synchronous stdin read that would + otherwise freeze the event loop). Declining raises click.Abort(), + which aborts the resume before any session.execute() can run. + - stdin is not a tty (CI, piped input, scripted flows): print the warning + and continue automatically. Scripted flows must never hang waiting for + input that will never arrive. + + This function is best-effort with respect to loading prior metadata: any + failure there (corrupt/missing metadata.json, invalid session_id, etc.) + is swallowed and treated as "nothing to compare against" -- this check + must never turn a resume that would otherwise have succeeded into a + failure. + + Args: + config: The (already resume-mode) SessionConfig for this session. + config.config is the fully-resolved active configuration (CLI + --provider/--model overrides are applied before this point, so + comparing against it automatically respects them). + session_id: The resolved session id being resumed. + console: Rich console for the warning. In JSON output modes, + create_initialized_session's caller has already redirected + console.file to stderr before create_initialized_session is + invoked, so this can never contaminate JSON stdout. + session: The already-created AmplifierSession (Steps 1-7.5 have run + by this point). On decline, this is cleaned up before raising + click.Abort so we never leave a half-initialized session behind. + + Raises: + click.Abort: if stdin is a tty and the user declines to continue. + """ + try: + prior_metadata = SessionStore().get_metadata(session_id) + except Exception: + logger.debug( + "Resume provider-mismatch check: could not load prior metadata " + "for session %s", + session_id, + exc_info=True, + ) + return + + if not isinstance(prior_metadata, dict): + return + + # Treat present-but-invalid provenance as unusable rather than guessing. + # A missing provider is valid for legacy metadata and retains model-only + # comparison, but null/list/numeric fields indicate malformed metadata. + for metadata_field in ("model", "provider"): + if metadata_field in prior_metadata and ( + not isinstance(prior_metadata[metadata_field], str) + or not prior_metadata[metadata_field] + ): + return + + prior_model = prior_metadata.get("model") + prior_provider = prior_metadata.get("provider") + + # Nothing recorded to compare against (e.g. corrupted/minimal recovered + # metadata, or a session saved before "model" was ever written) -- there + # is nothing to warn about. + if not prior_model and not prior_provider: + return + + provenance = get_effective_provider_model(config.config) + active_provider = provenance.provider + active_model = provenance.model + + if prior_provider: + # Full comparison: normalized provider identity AND model. + if ( + _normalize_provider_identity(prior_provider) + == _normalize_provider_identity(active_provider) + and prior_model == active_model + ): + return # Exact match -- silent, zero behavior change. + else: + # Feature-detection fallback for pre-existing sessions that predate + # the "provider" metadata field: compare model only. + if prior_model == active_model: + return # Silent -- nothing more to check without a prior provider. + + prior_label = f"{prior_provider or 'unknown'}/{prior_model or 'unknown'}" + active_label = f"{active_provider}/{active_model}" + + console.print( + f"[yellow]\u26a0 Provider/model mismatch:[/yellow] session was last " + f"written by {prior_label} \u2014 now resuming with {active_label}\n" + "[dim] Cross-provider replay can fail on provider-specific content " + "(e.g. thinking blocks).[/dim]" + ) + + if sys.stdin.isatty(): + # click.confirm() performs a synchronous, canonical-mode blocking + # stdin read (input()) with no executor offload. Calling it directly + # here would block the ENTIRE asyncio event loop thread until Enter + # is pressed. Offload to a worker thread, mirroring the existing + # pattern in main.py's KeyboardInterrupt handler and + # approval_provider.py's _get_user_input(). + if not await asyncio.to_thread( + click.confirm, + "Continue resuming with the new provider?", + default=False, + ): + console.print("[dim]Resume cancelled.[/dim]") + # Never leave a half-initialized session behind: Steps 1-7.5 have + # already created and partially wired up `session` by this point. + try: + await session.cleanup() + except Exception: + logger.debug( + "Session cleanup after declined resume failed", exc_info=True + ) + raise click.Abort() + _record_provider_mismatch(session_id, prior_label, active_label) + else: + # Non-interactive (CI, piped stdin, shadow environments): warn-only. + # Scripted flows must not hang waiting for input that can't arrive. + _record_provider_mismatch(session_id, prior_label, active_label) + + +def _record_provider_mismatch(session_id: str, prior: str, active: str) -> None: + """Best-effort audit trail for an accepted (or warned-through) mismatch. + + Mirrors _record_bundle_override() in commands/session.py: appends a + timestamped entry to a metadata list rather than overwriting anything. + Failures here are swallowed -- this is a diagnostics nicety, never a + condition that should fail the resume itself. + + Args: + session_id: The session whose metadata should record the mismatch. + prior: Display label for the provider/model that last wrote the + session (e.g. "provider-anthropic/claude-x"). + active: Display label for the provider/model now resuming it. + """ + try: + store = SessionStore() + metadata = store.get_metadata(session_id) + mismatches = list(metadata.get("provider_mismatches", [])) + mismatches.append( + { + "timestamp": datetime.now(UTC).isoformat(timespec="milliseconds"), + "previous": prior, + "resumed_with": active, + } + ) + store.update_metadata(session_id, {"provider_mismatches": mismatches}) + except Exception: + logger.debug( + "Could not record provider-mismatch audit trail for session %s", + session_id, + exc_info=True, + ) + + _CLEANUP_EVENTS: tuple[str, ...] = ( # PR #183 — cleanup-window diagnostic events emitted by app-cli's main.py only "cleanup:render_begin", diff --git a/amplifier_app_cli/session_spawner.py b/amplifier_app_cli/session_spawner.py index 4d1b0fa5..f1a2337b 100644 --- a/amplifier_app_cli/session_spawner.py +++ b/amplifier_app_cli/session_spawner.py @@ -14,6 +14,7 @@ from amplifier_foundation import RUNTIME_SKILL_OVERLAY_CAPABILITY from .agent_config import merge_configs +from .effective_config import get_effective_provider_model logger = logging.getLogger(__name__) @@ -867,6 +868,7 @@ async def _capture_completion(event: str, data: dict) -> HookResult: base = sub_session_id.rsplit("_", 1)[0] # Remove agent name child_span = base.rsplit("-", 1)[-1] # Get child_span (16 hex chars) + provenance = get_effective_provider_model(merged_config) metadata = { "session_id": sub_session_id, "parent_id": parent_session.session_id, @@ -874,6 +876,7 @@ async def _capture_completion(event: str, data: dict) -> HookResult: "agent_name": agent_name, "child_span": child_span, # For short_id resolution (first 8 chars = short_id) "created": datetime.now(UTC).isoformat(), + **provenance.as_metadata(), "config": merged_config, "agent_overlay": agent_config, "turn_count": 1, @@ -1366,6 +1369,7 @@ async def _capture_completion(event: str, data: dict) -> HookResult: updated_transcript = await context.get_messages() if context else [] metadata["turn_count"] = len(updated_transcript) metadata["last_updated"] = datetime.now(UTC).isoformat() + metadata.update(get_effective_provider_model(merged_config).as_metadata()) store.save(sub_session_id, updated_transcript, metadata) logger.debug( diff --git a/amplifier_app_cli/session_store.py b/amplifier_app_cli/session_store.py index cbbc0e3e..f7450b60 100644 --- a/amplifier_app_cli/session_store.py +++ b/amplifier_app_cli/session_store.py @@ -43,18 +43,20 @@ def is_top_level_session(session_id: str) -> bool: return "_" not in session_id -def extract_session_mode(metadata: dict) -> tuple[str | None, None]: +def extract_session_mode(metadata: object) -> tuple[str | None, None]: """Extract bundle name from session metadata. Sessions are created with a bundle (e.g., "foundation"). This function extracts the bundle name for session resumption. Args: - metadata: Session metadata dict containing "bundle" key + metadata: Parsed session metadata, normally a dict containing "bundle". + Malformed parseable values are ignored so resume can fall back to + the configured default bundle. Returns: - (bundle_name, None) tuple. Returns (None, None) if no bundle found, - allowing caller to fall back to configured default bundle. + (bundle_name, None) tuple. Returns (None, None) if no usable bundle is + found, allowing caller to fall back to the configured default bundle. Example: >>> extract_session_mode({"bundle": "bundle:foundation"}) @@ -64,8 +66,11 @@ def extract_session_mode(metadata: dict) -> tuple[str | None, None]: >>> extract_session_mode({}) (None, None) """ + if not isinstance(metadata, dict): + return (None, None) + bundle_value = metadata.get("bundle") - if bundle_value and bundle_value != "unknown": + if isinstance(bundle_value, str) and bundle_value and bundle_value != "unknown": if bundle_value.startswith(BUNDLE_PREFIX): return (bundle_value[len(BUNDLE_PREFIX) :], None) return (bundle_value, None) @@ -254,11 +259,15 @@ def _load_transcript(self, session_dir: Path) -> list: def _load_metadata(self, session_dir: Path) -> dict: """Load metadata with corruption recovery. + Parseable JSON scalars and arrays are malformed metadata, not usable + session state. Treat them as an empty mapping at this shared boundary so + all resume surfaces remain best-effort and silent. + Args: session_dir: Directory for this session Returns: - Metadata dictionary (empty dict if no metadata exists yet) + Metadata dictionary (empty dict if no usable metadata exists yet) """ metadata_file = session_dir / "metadata.json" backup_file = session_dir / "metadata.json.backup" @@ -271,7 +280,8 @@ def _load_metadata(self, session_dir: Path) -> dict: if metadata_file.exists(): try: with open(metadata_file, encoding="utf-8") as f: - return json.load(f) + metadata = json.load(f) + return metadata if isinstance(metadata, dict) else {} except (OSError, json.JSONDecodeError) as e: logger.warning(f"Failed to load metadata, trying backup: {e}") @@ -281,7 +291,7 @@ def _load_metadata(self, session_dir: Path) -> dict: with open(backup_file, encoding="utf-8") as f: metadata = json.load(f) logger.info("Loaded metadata from backup") - return metadata + return metadata if isinstance(metadata, dict) else {} except (OSError, json.JSONDecodeError) as e: logger.error(f"Backup also corrupted: {e}") diff --git a/tests/test_resume_provider_metadata_write.py b/tests/test_resume_provider_metadata_write.py new file mode 100644 index 00000000..5f1852db --- /dev/null +++ b/tests/test_resume_provider_metadata_write.py @@ -0,0 +1,386 @@ +"""Tests for write-side provider persistence in session metadata. + +Wave 2 of the cross-provider resume hardening set (amplifier-support#208): +this repo's metadata-writing code -- both the interactive `_save_session()` +closure inside `interactive_chat()` and the single-shot save block inside +`execute_single()` -- must persist the active `"provider"` identity +alongside the existing `"model"` field. Without this, the resume-time +mismatch check (session_runner._warn_on_resume_provider_mismatch) has +nothing new to compare against for sessions saved going forward. + +Both write sites derive `"provider"` and `"model"` as one pair from +`get_effective_config_summary()` -- the same function the read-side check uses +at resume -- so metadata stores the provider module id and bare model value +that the comparison expects (e.g. `"provider-anthropic"` and `"claude-x"`). +""" + +from __future__ import annotations + +from contextlib import nullcontext +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +_MODULE = "amplifier_app_cli.main" + + +def _make_mock_hooks() -> MagicMock: + """Return a mock hooks registry whose emit() is an inert AsyncMock.""" + mock = MagicMock() + mock.emit = AsyncMock(return_value=None) + return mock + + +def _make_mock_session(hooks: MagicMock, providers: dict | None = None) -> MagicMock: + """Return a minimal mock AmplifierSession suitable for execute_single().""" + mock_ctx = MagicMock() + mock_ctx.get_messages = AsyncMock(return_value=[{"role": "user", "content": "Hi"}]) + + def _coordinator_get(key: str): + if key == "hooks": + return hooks + if key == "context": + return mock_ctx + if key == "providers": + return providers if providers is not None else {} + return None + + mock_session = MagicMock() + mock_session.session_id = "test-session-id" + mock_session.execute = AsyncMock(return_value="Hello!") + mock_session.coordinator = MagicMock() + mock_session.coordinator.get = _coordinator_get + mock_session.coordinator.get_capability.return_value = None + mock_session.coordinator.session_state = {} + mock_session.coordinator.cancellation = MagicMock() + mock_session.coordinator.cancellation.is_cancelled = False + mock_session.coordinator.cancellation.is_immediate = False + return mock_session + + +def _make_mock_initialized(session: MagicMock) -> MagicMock: + """Return a minimal InitializedSession mock wrapping *session*.""" + mock = MagicMock() + mock.session = session + mock.session_id = "test-session-id" + mock.cleanup = AsyncMock() + return mock + + +class TestExecuteSingleWritesProvider: + """execute_single()'s final metadata-save block persists 'provider'.""" + + @pytest.mark.asyncio + async def test_metadata_writes_bare_effective_pair_and_resumes_silently( + self, tmp_path: Path + ): + """Single-shot metadata round-trips through resume without a false mismatch.""" + from amplifier_app_cli.main import execute_single + from amplifier_app_cli.session_runner import ( + SessionConfig, + _warn_on_resume_provider_mismatch, + ) + + hooks = _make_mock_hooks() + runtime_provider = MagicMock() + runtime_provider.model = "claude-x" + session = _make_mock_session(hooks, providers={"anthropic": runtime_provider}) + initialized = _make_mock_initialized(session) + + config = { + "providers": [ + { + "module": "provider-anthropic", + "config": { + "model": "claude-x", + "default_model": "stale-default", + "priority": 0, + }, + } + ] + } + + with ( + patch( + f"{_MODULE}.create_initialized_session", + new=AsyncMock(return_value=initialized), + ), + patch(f"{_MODULE}.SessionStore") as MockStore, + patch(f"{_MODULE}.console"), + patch( + f"{_MODULE}.process_runtime_mentions", + new=AsyncMock(side_effect=lambda _s, p: p), + ), + ): + store_instance = MockStore.return_value + store_instance.get_metadata.return_value = {} + store_instance.save.return_value = None + + await execute_single( + prompt="Hi", + config=config, + search_paths=[tmp_path], + verbose=False, + output_format="text", + bundle_name="bundle:test", + ) + + store_instance.save.assert_called_once() + _saved_id, _saved_messages, saved_metadata = store_instance.save.call_args[0] + assert saved_metadata["provider"] == "provider-anthropic" + assert saved_metadata["model"] == "claude-x" + assert saved_metadata["model"] != "anthropic/claude-x" + + # Feed the write-side result directly to the read-side resume check. The + # same effective config must be silent: no warning and no confirmation. + resume_config = SessionConfig( + config=config, + search_paths=[], + verbose=False, + bundle_name="bundle:test", + initial_transcript=[{"role": "user", "content": "Hi"}], + ) + resume_console = MagicMock() + resume_session = MagicMock() + with ( + patch("amplifier_app_cli.session_runner.SessionStore") as ResumeStore, + patch("amplifier_app_cli.session_runner.click.confirm") as mock_confirm, + ): + ResumeStore.return_value.get_metadata.return_value = saved_metadata + await _warn_on_resume_provider_mismatch( + resume_config, + "test-session-id", + resume_console, + resume_session, + ) + + resume_console.print.assert_not_called() + mock_confirm.assert_not_called() + + @pytest.mark.asyncio + async def test_metadata_preserves_existing_fields_alongside_provider( + self, tmp_path: Path + ): + """Existing metadata fields (name, description, ...) survive the save, + with 'provider' added alongside them -- not replacing them.""" + from amplifier_app_cli.main import execute_single + + hooks = _make_mock_hooks() + session = _make_mock_session(hooks) + initialized = _make_mock_initialized(session) + + config = { + "providers": [ + { + "module": "provider-openai", + "config": {"default_model": "gpt-x", "priority": 0}, + } + ] + } + + with ( + patch( + f"{_MODULE}.create_initialized_session", + new=AsyncMock(return_value=initialized), + ), + patch(f"{_MODULE}.SessionStore") as MockStore, + patch(f"{_MODULE}.console"), + patch( + f"{_MODULE}.process_runtime_mentions", + new=AsyncMock(side_effect=lambda _s, p: p), + ), + ): + store_instance = MockStore.return_value + store_instance.get_metadata.return_value = { + "name": "my-named-session", + "description": "a test session", + } + store_instance.save.return_value = None + + await execute_single( + prompt="Hi", + config=config, + search_paths=[tmp_path], + verbose=False, + output_format="text", + bundle_name="bundle:test", + ) + + _saved_id, _saved_messages, saved_metadata = store_instance.save.call_args[0] + assert saved_metadata["provider"] == "provider-openai" + assert saved_metadata["model"] == "gpt-x" + assert saved_metadata["name"] == "my-named-session" + assert saved_metadata["description"] == "a test session" + + +class TestInteractiveChatWritesProviderPair: + """interactive_chat() persists provider and model from one effective summary.""" + + @pytest.mark.asyncio + async def test_metadata_uses_effective_pair_not_first_provider( + self, tmp_path: Path + ): + """The interactive write site cannot combine two different providers.""" + from amplifier_app_cli.main import interactive_chat + + hooks = _make_mock_hooks() + session = _make_mock_session(hooks) + initialized = _make_mock_initialized(session) + prompt_session = MagicMock() + prompt_session.prompt_async = AsyncMock(side_effect=EOFError) + steering_manager = MagicMock() + steering_manager.run = AsyncMock(return_value=None) + + # The first list entry deliberately disagrees with the effective summary. + # This catches the old implementation that read model from providers[0] + # while reading provider from the priority-resolved summary. + config = { + "providers": [ + { + "module": "provider-openai", + "config": {"default_model": "wrong-first-model", "priority": 10}, + }, + { + "module": "provider-anthropic", + "config": {"default_model": "claude-effective", "priority": 0}, + }, + ] + } + with ( + patch( + f"{_MODULE}.create_initialized_session", + new=AsyncMock(return_value=initialized), + ), + patch(f"{_MODULE}.SessionStore") as MockStore, + patch(f"{_MODULE}.console"), + patch(f"{_MODULE}._create_prompt_session", return_value=prompt_session), + patch( + f"{_MODULE}.patch_stdout", + side_effect=lambda *args, **kwargs: nullcontext(), + ), + patch(f"{_MODULE}.signal.signal", return_value=MagicMock()), + patch(f"{_MODULE}.close_dedicated_tty_input"), + patch( + f"{_MODULE}.process_runtime_mentions", + new=AsyncMock(side_effect=lambda _s, p: p), + ), + patch("amplifier_app_cli.incremental_save.register_incremental_save"), + patch("amplifier_app_cli.goal_progress_hook.register_goal_progress_hook"), + patch( + "amplifier_app_cli.steering_input.SteeringInputManager", + return_value=steering_manager, + ), + patch("amplifier_app_cli.ui.render_message"), + patch( + "amplifier_foundation.session.diagnose_transcript", + return_value={"status": "ok"}, + ), + ): + store_instance = MockStore.return_value + store_instance.get_metadata.return_value = {"name": "keep-me"} + + await interactive_chat( + config=config, + search_paths=[tmp_path], + verbose=False, + bundle_name="bundle:test", + initial_prompt="Hi", + initial_transcript=[], + ) + + store_instance.save.assert_called_once() + _saved_id, _saved_messages, saved_metadata = store_instance.save.call_args[0] + assert saved_metadata["provider"] == "provider-anthropic" + assert saved_metadata["model"] == "claude-effective" + assert saved_metadata["name"] == "keep-me" + + +class TestCanonicalProvenanceAndIncrementalSave: + """All metadata writers share priority and model-resolution semantics.""" + + def test_explicit_model_precedes_default_model_in_summary_and_provenance(self): + from amplifier_app_cli.effective_config import get_effective_config_summary + from amplifier_app_cli.effective_config import get_effective_provider_model + + config = { + "providers": [ + { + "module": "provider-anthropic", + "config": { + "model": "explicit-model", + "default_model": "fallback-model", + }, + } + ] + } + + provenance = get_effective_provider_model(config) + summary = get_effective_config_summary(config, "bundle:test") + + assert provenance.as_metadata() == { + "provider": "provider-anthropic", + "model": "explicit-model", + } + assert summary.provider_module == provenance.provider + assert summary.model == provenance.model + + @pytest.mark.asyncio + async def test_incremental_save_persists_priority_pair_and_preserves_metadata( + self, tmp_path: Path + ): + from amplifier_app_cli.incremental_save import IncrementalSaveHook + from amplifier_app_cli.session_store import SessionStore + + messages = [ + {"role": "user", "content": "Hi"}, + {"role": "assistant", "content": "Hello"}, + ] + context = MagicMock() + context.get_messages = AsyncMock(return_value=messages) + session = MagicMock() + session.coordinator.get.return_value = context + store = SessionStore(base_dir=tmp_path) + store.save( + "incremental-session", + [], + { + "name": "preserved-name", + "description": "preserved-description", + "created": "2025-01-01T00:00:00+00:00", + }, + ) + config = { + "providers": [ + { + "module": "provider-openai", + "config": {"model": "wrong-first", "priority": 10}, + }, + { + "module": "provider-anthropic", + "config": { + "model": "priority-model", + "default_model": "stale-default", + "priority": 0, + }, + }, + ] + } + hook = IncrementalSaveHook( + session, + store, + "incremental-session", + "bundle:test", + config, + ) + + result = await hook.on_tool_post("tool:post", {"tool_name": "read_file"}) + _transcript, metadata = store.load("incremental-session") + + assert result.action == "continue" + assert metadata["provider"] == "provider-anthropic" + assert metadata["model"] == "priority-model" + assert metadata["name"] == "preserved-name" + assert metadata["description"] == "preserved-description" + assert metadata["created"] == "2025-01-01T00:00:00+00:00" + assert metadata["incremental"] is True diff --git a/tests/test_session_runner.py b/tests/test_session_runner.py index b7b528c6..91c3d9c6 100644 --- a/tests/test_session_runner.py +++ b/tests/test_session_runner.py @@ -575,9 +575,7 @@ def _configurator_patches(mock_sess): new_callable=AsyncMock, return_value=mock_sess, ), - patch( - "amplifier_app_cli.commands.init.check_first_run", return_value=False - ), + patch("amplifier_app_cli.commands.init.check_first_run", return_value=False), patch( "amplifier_app_cli.project_utils.get_project_slug", return_value="test-slug", @@ -852,3 +850,437 @@ async def test_configurator_none_on_general_exception(self, caplog): r.message for r in caplog.records if r.levelno == logging.WARNING ] assert warning_messages, "Expected a warning but none was logged" + + +# --------------------------------------------------------------------------- +# Resume-time provider/model mismatch check (amplifier-support#208, Wave 2) +# --------------------------------------------------------------------------- + +import click as _click # noqa: E402 (grouped here to stay close to its tests) + +from amplifier_app_cli.effective_config import EffectiveProviderModel # noqa: E402 +from amplifier_app_cli.session_runner import ( # noqa: E402 + _normalize_provider_identity, + _warn_on_resume_provider_mismatch, +) + + +def _make_summary( + provider_module: str = "provider-anthropic", model: str = "claude-x" +) -> EffectiveProviderModel: + """Return canonical provenance for mismatch-check tests.""" + return EffectiveProviderModel(provider=provider_module, model=model) + + +class TestNormalizeProviderIdentity: + """Unit tests for _normalize_provider_identity().""" + + def test_strips_provider_prefix(self): + assert _normalize_provider_identity("provider-anthropic") == "anthropic" + + def test_bare_name_passes_through_unchanged(self): + assert _normalize_provider_identity("anthropic") == "anthropic" + + def test_case_insensitive(self): + assert _normalize_provider_identity("Provider-Anthropic") == "anthropic" + assert _normalize_provider_identity("ANTHROPIC") == "anthropic" + + def test_none_and_empty_string_normalize_to_empty(self): + assert _normalize_provider_identity(None) == "" + assert _normalize_provider_identity("") == "" + + +class TestWarnOnResumeProviderMismatch: + """Unit tests for _warn_on_resume_provider_mismatch() -- the read-side policy.""" + + @staticmethod + def _resume_config() -> SessionConfig: + return _make_session_config( + initial_transcript=[{"role": "user", "content": "hi"}] + ) + + @pytest.mark.asyncio + async def test_mismatch_tty_prints_warning_and_confirms_then_proceeds(self): + """mismatch + tty -> warning printed AND confirm invoked; accept proceeds.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + session.cleanup = AsyncMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="provider-openai", model="gpt-x" + ), + ), + patch("sys.stdin.isatty", return_value=True), + patch(f"{_MODULE}.click.confirm", return_value=True) as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = { + "model": "claude-x", + "provider": "provider-anthropic", + } + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_called() + mock_confirm.assert_called_once() + session.cleanup.assert_not_called() + printed = " ".join(str(c) for c in console.print.call_args_list) + assert "provider-anthropic/claude-x" in printed + assert "provider-openai/gpt-x" in printed + + @pytest.mark.asyncio + async def test_mismatch_tty_decline_aborts_cleanly(self): + """mismatch + tty + decline -> click.Abort raised, session cleaned up.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + session.cleanup = AsyncMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="provider-openai", model="gpt-x" + ), + ), + patch("sys.stdin.isatty", return_value=True), + patch(f"{_MODULE}.click.confirm", return_value=False), + ): + MockStore.return_value.get_metadata.return_value = { + "model": "claude-x", + "provider": "provider-anthropic", + } + with pytest.raises(_click.Abort): + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + session.cleanup.assert_called_once() + + @pytest.mark.asyncio + async def test_mismatch_non_tty_warns_and_continues_without_confirm(self): + """mismatch + non-tty -> warning printed, NO confirm, proceeds automatically.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + session.cleanup = AsyncMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="provider-openai", model="gpt-x" + ), + ), + patch("sys.stdin.isatty", return_value=False), + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = { + "model": "claude-x", + "provider": "provider-anthropic", + } + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_called() + mock_confirm.assert_not_called() + session.cleanup.assert_not_called() + + @pytest.mark.asyncio + async def test_provider_and_model_match_is_completely_silent(self): + """Exact provider+model match -> no warning, no confirm (zero behavior change).""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="provider-anthropic", model="claude-x" + ), + ), + patch("sys.stdin.isatty", return_value=True), + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = { + "model": "claude-x", + "provider": "provider-anthropic", + } + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_not_called() + mock_confirm.assert_not_called() + + @pytest.mark.asyncio + async def test_missing_provider_field_model_differs_falls_back_to_model_only(self): + """Pre-existing session with no 'provider' field: model-only fallback still warns.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="provider-openai", model="gpt-x" + ), + ), + patch("sys.stdin.isatty", return_value=False), + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + # No "provider" key at all -- simulates a session saved before this PR. + MockStore.return_value.get_metadata.return_value = {"model": "claude-x"} + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_called() + mock_confirm.assert_not_called() + + @pytest.mark.asyncio + async def test_missing_provider_field_model_matches_is_silent(self): + """Pre-existing session, no 'provider' field, model matches -> silent.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="provider-anthropic", model="claude-x" + ), + ), + patch("sys.stdin.isatty", return_value=True), + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = {"model": "claude-x"} + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_not_called() + mock_confirm.assert_not_called() + + @pytest.mark.asyncio + async def test_provider_normalization_prevents_false_positive(self): + """Stored 'provider-anthropic' vs active bare 'anthropic' -> normalized, no warning.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", + return_value=_make_summary( + provider_module="anthropic", model="claude-x" + ), + ), + patch("sys.stdin.isatty", return_value=True), + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = { + "model": "claude-x", + "provider": "provider-anthropic", + } + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_not_called() + mock_confirm.assert_not_called() + + @pytest.mark.asyncio + async def test_no_prior_metadata_at_all_is_silent(self): + """Empty prior metadata (nothing to compare against) -> silent.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary() + ), + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = {} + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_not_called() + mock_confirm.assert_not_called() + + @pytest.mark.asyncio + async def test_prior_metadata_load_failure_is_silent_best_effort(self): + """Loading prior metadata fails (e.g. corrupted session) -> silent no-op.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch( + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary() + ), + ): + MockStore.return_value.get_metadata.side_effect = FileNotFoundError("nope") + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_not_called() + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "metadata", + [ + None, + [], + "metadata", + {"model": None, "provider": "provider-anthropic"}, + {"model": [], "provider": "provider-anthropic"}, + {"model": 42, "provider": "provider-anthropic"}, + {"model": "claude-x", "provider": None}, + {"model": "claude-x", "provider": []}, + {"model": "claude-x", "provider": 42}, + ], + ) + async def test_parseable_malformed_metadata_is_silently_ignored(self, metadata): + """Malformed JSON values never make the best-effort guard fail or warn.""" + cfg = self._resume_config() + console = MagicMock() + session = MagicMock() + + with ( + patch(f"{_MODULE}.SessionStore") as MockStore, + patch(f"{_MODULE}.get_effective_provider_model") as mock_provenance, + patch(f"{_MODULE}.click.confirm") as mock_confirm, + ): + MockStore.return_value.get_metadata.return_value = metadata + await _warn_on_resume_provider_mismatch(cfg, "sess-1", console, session) + + console.print.assert_not_called() + mock_confirm.assert_not_called() + mock_provenance.assert_not_called() + + +class TestSessionResumeCommandMalformedMetadata: + """Exercise the real command -> shared metadata reader resume path.""" + + @pytest.mark.parametrize( + "raw_metadata", + [[], "metadata", None, {"bundle": ["anchors"]}], + ) + def test_session_resume_silently_falls_back_for_malformed_metadata( + self, tmp_path, monkeypatch, raw_metadata + ): + import json + + import click + from click.testing import CliRunner + + from amplifier_app_cli.commands.session import register_session_commands + from amplifier_app_cli.session_store import SessionStore + + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + session_id = "resume-malformed-metadata" + session_dir = store.base_dir / session_id + session_dir.mkdir(parents=True) + (session_dir / "metadata.json").write_text( + json.dumps(raw_metadata), encoding="utf-8" + ) + (session_dir / "transcript.jsonl").write_text("", encoding="utf-8") + + cli = click.Group() + interactive_chat = AsyncMock(return_value=None) + execute_single = AsyncMock(return_value=None) + resolved_bundles: list[str | None] = [] + + def fake_resolve_config(*, bundle_name, **_kwargs): + resolved_bundles.append(bundle_name) + return ({}, None) + + register_session_commands( + cli, + interactive_chat=interactive_chat, + execute_single=execute_single, + get_module_search_paths=lambda: [], + ) + + with ( + patch( + "amplifier_app_cli.commands.session.resolve_config", + side_effect=fake_resolve_config, + ), + patch("amplifier_app_cli.commands.session.AppSettings"), + patch("amplifier_app_cli.commands.session.get_project_slug", return_value="p"), + patch("amplifier_app_cli.commands.init.check_first_run", return_value=False), + ): + result = CliRunner().invoke( + cli, ["session", "resume", session_id, "--no-history"] + ) + + assert result.exit_code == 0, result.output + assert "Error resuming session" not in result.output + assert resolved_bundles == [None] + interactive_chat.assert_awaited_once() + assert interactive_chat.await_args.kwargs["initial_transcript"] == [] + + +class TestProviderMismatchCheckWiredIntoChokepoint: + """Integration: create_initialized_session gates the check on config.is_resume. + + This is the single chokepoint every resume surface passes through + (amplifier run --resume, amplifier session resume, amplifier resume, + amplifier continue all funnel through create_initialized_session). + """ + + @pytest.mark.asyncio + async def test_check_never_runs_for_a_new_non_resume_session(self): + """is_resume=False (new session) -> the mismatch check must never run.""" + from contextlib import ExitStack + + mock_sess = _make_mock_session() + cfg = _make_session_config() # no initial_transcript -> is_resume is False + console = MagicMock() + + with ExitStack() as stack: + for p in _configurator_patches(mock_sess): + stack.enter_context(p) + mock_check = stack.enter_context( + patch( + f"{_MODULE}._warn_on_resume_provider_mismatch", + new_callable=AsyncMock, + ) + ) + await create_initialized_session(cfg, console) + + mock_check.assert_not_called() + + @pytest.mark.asyncio + async def test_check_runs_for_a_resume_session_with_correct_args(self): + """is_resume=True (resume) -> the mismatch check runs with (config, session_id, console, session).""" + from contextlib import ExitStack + + mock_sess = _make_mock_session() + cfg = _make_session_config( + initial_transcript=[{"role": "user", "content": "hi"}] + ) + console = MagicMock() + + with ExitStack() as stack: + for p in _configurator_patches(mock_sess): + stack.enter_context(p) + mock_check = stack.enter_context( + patch( + f"{_MODULE}._warn_on_resume_provider_mismatch", + new_callable=AsyncMock, + ) + ) + result = await create_initialized_session(cfg, console) + + mock_check.assert_called_once() + call_args = mock_check.call_args[0] + assert call_args[0] is cfg + assert call_args[2] is console + assert call_args[3] is result.session diff --git a/tests/test_session_spawner.py b/tests/test_session_spawner.py index e304ea2d..c53d1151 100644 --- a/tests/test_session_spawner.py +++ b/tests/test_session_spawner.py @@ -2192,6 +2192,192 @@ async def test_factory_content_is_mention_expanded(self, tmp_path, monkeypatch): ) +class TestDelegatedProviderModelMetadata: + """Delegated spawn/resume writers persist canonical provenance.""" + + @staticmethod + def _coordinator(): + from unittest.mock import AsyncMock, MagicMock + + capabilities: dict = {} + + class FakeHooks: + def register(self, _event, _handler, priority=0, name=None): + return lambda: None + + async def emit(self, _event, _data): + return None + + context = AsyncMock() + context.get_messages = AsyncMock( + return_value=[{"role": "user", "content": "delegated"}] + ) + context.add_message = AsyncMock() + + coordinator = MagicMock() + coordinator.config = {} + coordinator.display_system = MagicMock() + coordinator.approval_system = MagicMock() + coordinator.cancellation = MagicMock() + coordinator.cancellation.register_child = MagicMock() + coordinator.cancellation.unregister_child = MagicMock() + coordinator.mount = AsyncMock() + coordinator.collect_contributions = AsyncMock(return_value=[]) + coordinator.register_capability.side_effect = capabilities.__setitem__ + coordinator.get_capability.side_effect = capabilities.get + coordinator.get.side_effect = lambda name: { + "hooks": FakeHooks(), + "context": context, + }.get(name) + return coordinator + + @staticmethod + def _session(coordinator, session_id): + from unittest.mock import AsyncMock, MagicMock + + session = MagicMock() + session.coordinator = coordinator + session.session_id = session_id + session.initialize = AsyncMock() + session.execute = AsyncMock(return_value="response") + session.cleanup = AsyncMock() + return session + + async def test_spawn_writer_uses_priority_pair_and_top_level_model( + self, tmp_path, monkeypatch + ): + from unittest.mock import MagicMock, patch + + from amplifier_app_cli.session_runner import ( + SessionConfig, + _warn_on_resume_provider_mismatch, + ) + from amplifier_app_cli.session_spawner import spawn_sub_session + + monkeypatch.setenv("HOME", str(tmp_path)) + parent = MagicMock() + parent.coordinator = self._coordinator() + parent.session_id = "parent" + parent.trace_id = "trace" + parent.loader = None + parent.config = { + "session": {"orchestrator": "loop-basic", "context": "context-simple"}, + "providers": [ + { + "module": "provider-first", + "config": {"priority": 50, "default_model": "wrong"}, + }, + { + "module": "provider-priority", + "config": { + "priority": 1, + "model": "runtime-model", + "default_model": "stale-default", + }, + }, + ], + } + child = self._session(self._coordinator(), "delegated-spawn") + + with ( + patch( + "amplifier_app_cli.session_spawner.AmplifierSession", + return_value=child, + ), + patch("amplifier_app_cli.paths.create_foundation_resolver"), + ): + await spawn_sub_session( + agent_name="worker", + instruction="work", + parent_session=parent, + agent_configs={"worker": {"description": "worker"}}, + sub_session_id="delegated-spawn", + session_metadata={"correlation_id": "keep-me"}, + ) + + _transcript, metadata = SessionStore().load("delegated-spawn") + assert metadata["provider"] == "provider-priority" + assert metadata["model"] == "runtime-model" + assert metadata["config"]["session"]["metadata"] == { + "correlation_id": "keep-me" + } + + resume_config = SessionConfig( + config=metadata["config"], + search_paths=[], + verbose=False, + session_id="delegated-spawn", + initial_transcript=[{"role": "user", "content": "delegated"}], + ) + console = MagicMock() + with patch("amplifier_app_cli.session_runner.click.confirm") as confirm: + await _warn_on_resume_provider_mismatch( + resume_config, "delegated-spawn", console, child + ) + console.print.assert_not_called() + confirm.assert_not_called() + + async def test_resumed_delegate_writer_updates_pair_and_preserves_metadata( + self, tmp_path, monkeypatch + ): + from unittest.mock import patch + + from amplifier_app_cli.session_spawner import resume_sub_session + + monkeypatch.setenv("HOME", str(tmp_path)) + store = SessionStore() + config = { + "session": {"orchestrator": "loop-basic", "context": "context-simple"}, + "providers": [ + { + "module": "provider-lower", + "config": {"priority": 9, "default_model": "wrong"}, + }, + { + "module": "provider-effective", + "config": { + "priority": 0, + "model": "top-level-model", + "default_model": "stale-default", + }, + }, + ], + } + store.save( + "delegated-resume", + [{"role": "user", "content": "before"}], + { + "config": config, + "provider": "provider-old", + "model": "old-model", + "custom": {"preserve": True}, + "parent_id": "parent", + "agent_name": "worker", + "trace_id": "trace", + }, + ) + child = self._session(self._coordinator(), "delegated-resume") + + with ( + patch( + "amplifier_app_cli.session_spawner.AmplifierSession", + return_value=child, + ), + patch("amplifier_app_cli.paths.create_foundation_resolver"), + patch("amplifier_app_cli.ui.CLIApprovalSystem"), + patch("amplifier_app_cli.ui.CLIDisplaySystem"), + ): + await resume_sub_session("delegated-resume", "continue") + + transcript, metadata = store.load("delegated-resume") + assert transcript + assert metadata["provider"] == "provider-effective" + assert metadata["model"] == "top-level-model" + assert metadata["custom"] == {"preserve": True} + assert metadata["parent_id"] == "parent" + assert metadata["agent_name"] == "worker" + + class TestResumeMentionExpansion: """@-mentions in resume instructions must be expanded before reaching the LLM. diff --git a/tests/test_session_store_sanitization.py b/tests/test_session_store_sanitization.py index 47b6e968..20fa5d99 100644 --- a/tests/test_session_store_sanitization.py +++ b/tests/test_session_store_sanitization.py @@ -1,9 +1,33 @@ """Test session store message sanitization for extended thinking.""" +import json import tempfile from pathlib import Path -from amplifier_app_cli.session_store import SessionStore +from amplifier_app_cli.session_store import SessionStore, extract_session_mode + + +def test_malformed_parseable_metadata_is_normalized_at_resume_read_boundary(tmp_path): + """All resume readers receive a mapping and unusable bundle values are ignored.""" + store = SessionStore(tmp_path) + + for index, raw_metadata in enumerate(([], "metadata", None)): + session_id = f"malformed-{index}" + session_dir = tmp_path / session_id + session_dir.mkdir() + (session_dir / "metadata.json").write_text( + json.dumps(raw_metadata), encoding="utf-8" + ) + + transcript, metadata = store.load(session_id) + + assert transcript == [] + assert metadata == {} + assert extract_session_mode(metadata) == (None, None) + + +def test_extract_session_mode_ignores_non_string_bundle(): + assert extract_session_mode({"bundle": ["anchors"]}) == (None, None) def message_matches_ignoring_timestamp(loaded: dict, original: dict) -> bool: