From 44b8a6310440f856747a820c3086474b12c27bf2 Mon Sep 17 00:00:00 2001 From: "Michael J. Jabbour" Date: Wed, 15 Jul 2026 07:35:42 -0400 Subject: [PATCH 1/4] feat: warn and confirm at resume when active provider/model differs from session's last writer Wave 2 of the cross-provider resume hardening set. Investigation found amplifier-core persists no session files (kernel emits events only; its DESIGN_PHILOSOPHY.md explicitly excludes provider selection/logging policy from the kernel) -- metadata.json (including the existing 'model' field) is owned entirely by this repo. The originally-planned amplifier-core PR is therefore skipped; this PR carries both halves: 1. Write-side mechanism: persist the active 'provider' identity into session metadata alongside the existing 'model' (main.py, both the interactive _save_session() closure and execute_single()'s save block). Both derive it from get_effective_config_summary().provider_module -- the same function the read-side check uses -- so write and read agree. 2. Read-side policy: at the single chokepoint every resume surface passes through (create_initialized_session, after transcript/cost restore and before the first session.execute()), compare the active provider/model against whichever provider/model last wrote the session's metadata. - Match, or no prior metadata: completely silent, zero behavior change. - Mismatch + tty: warn, then require explicit confirmation (click.confirm offloaded via asyncio.to_thread, matching the existing KeyboardInterrupt-handler pattern). Decline cleans up the already-created session and raises click.Abort -- no half-initialized session left behind. - Mismatch + non-tty (CI/piped/scripted): warn-only, proceed automatically -- scripted flows must never hang on input. - Feature-detected fallback: pre-existing sessions (no 'provider' field) compare model-only. No amplifier-core version bump required -- metadata is read directly off disk. - Provider identity is normalized (module id vs. bare name) before comparison to avoid false positives. - Accepted/warned-through mismatches are recorded to a 'provider_mismatches' audit trail, mirroring _record_bundle_override(). Addresses microsoft-amplifier/amplifier-support#208 -- implements Option A (warn at resume). Option B's provider-edge normalization is delivered by provider-anthropic#72. --- amplifier_app_cli/main.py | 18 + amplifier_app_cli/session_runner.py | 225 ++++++++++++ tests/test_resume_provider_metadata_write.py | 173 +++++++++ tests/test_session_runner.py | 349 ++++++++++++++++++- 4 files changed, 762 insertions(+), 3 deletions(-) create mode 100644 tests/test_resume_provider_metadata_write.py diff --git a/amplifier_app_cli/main.py b/amplifier_app_cli/main.py index de93a2ed..81abed98 100644 --- a/amplifier_app_cli/main.py +++ b/amplifier_app_cli/main.py @@ -2892,6 +2892,17 @@ def _extract_model_name() -> str: ) return "unknown" + # Helper to extract the active provider identity from config. + # + # Uses get_effective_config_summary() -- the same function the resume-time + # mismatch check in session_runner.py uses to compute the ACTIVE provider -- + # so the value written here and the value compared at resume are always + # derived the same way. Returns the provider module id (e.g. + # "provider-anthropic"); session_runner._normalize_provider_identity() + # handles comparing this against older/bare-name forms. + def _extract_provider_identity() -> str: + return get_effective_config_summary(config, bundle_name).provider_module + # Helper to save session after each turn async def _save_session(): context = session.coordinator.get("context") @@ -2908,6 +2919,7 @@ async def _save_session(): ), "bundle": bundle_name, "model": _extract_model_name(), + "provider": _extract_provider_identity(), "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()), @@ -3655,6 +3667,12 @@ def _goal_sigint_handler(signum, frame): ), "bundle": bundle_name, "model": model_name, + # Same source (get_effective_config_summary) as the resume-time + # mismatch check in session_runner.py -- see _extract_provider_identity() + # in interactive_chat() above for the parallel single-shot-vs-chat note. + "provider": get_effective_config_summary( + config, bundle_name + ).provider_module, "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..86e60413 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_config_summary 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,215 @@ 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: str | None) -> 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 value: + return "" + normalized = value.strip().lower() + if normalized.startswith("provider-"): + normalized = normalized[len("provider-") :] + return normalized + + +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 + + 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 + + summary = get_effective_config_summary(config.config, config.bundle_name) + active_provider = summary.provider_module + active_model = summary.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/tests/test_resume_provider_metadata_write.py b/tests/test_resume_provider_metadata_write.py new file mode 100644 index 00000000..d1af5be3 --- /dev/null +++ b/tests/test_resume_provider_metadata_write.py @@ -0,0 +1,173 @@ +"""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"` from `get_effective_config_summary()` +(`.provider_module`) -- the same function the read-side check uses to +compute the ACTIVE provider at resume -- so write-side and read-side always +agree on what "provider identity" means and how it's shaped (module id form, +e.g. "provider-anthropic"). +""" + +from __future__ import annotations + +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.cancellation = MagicMock() + mock_session.coordinator.cancellation.is_cancelled = 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_includes_provider_matching_config(self, tmp_path: Path): + """The saved metadata's 'provider' matches the resolved config's provider_module.""" + 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-anthropic", + "config": {"default_model": "claude-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 = {} + 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.get("provider") == "provider-anthropic" + # Sanity: 'model' is still written too (pre-existing field, untouched by this PR). + assert "model" in saved_metadata + + @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["name"] == "my-named-session" + assert saved_metadata["description"] == "a test session" diff --git a/tests/test_session_runner.py b/tests/test_session_runner.py index b7b528c6..c3b945c7 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,348 @@ 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 EffectiveConfigSummary # 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" +) -> EffectiveConfigSummary: + """Return a minimal EffectiveConfigSummary for mismatch-check tests.""" + return EffectiveConfigSummary( + config_source="bundle:test", + provider_name=provider_module.replace("provider-", "").title(), + provider_module=provider_module, + model=model, + orchestrator="loop-basic", + tool_count=0, + hook_count=0, + ) + + +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_config_summary", + 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_config_summary", + 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_config_summary", + 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_config_summary", + 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_config_summary", + 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_config_summary", + 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_config_summary", + 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_config_summary", 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_config_summary", 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() + + +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 From a3d86982ecc1075108475d5fe624b65ddd9a18a2 Mon Sep 17 00:00:00 2001 From: Sam Schillace Date: Thu, 6 Aug 2026 15:57:24 -0700 Subject: [PATCH 2/4] fix: keep resume provider metadata model consistent --- amplifier_app_cli/main.py | 43 ++---- tests/test_resume_provider_metadata_write.py | 150 +++++++++++++++++-- 2 files changed, 151 insertions(+), 42 deletions(-) diff --git a/amplifier_app_cli/main.py b/amplifier_app_cli/main.py index 81abed98..0cf51f0c 100644 --- a/amplifier_app_cli/main.py +++ b/amplifier_app_cli/main.py @@ -2881,28 +2881,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 extract the active provider identity from config. - # - # Uses get_effective_config_summary() -- the same function the resume-time - # mismatch check in session_runner.py uses to compute the ACTIVE provider -- - # so the value written here and the value compared at resume are always - # derived the same way. Returns the provider module id (e.g. - # "provider-anthropic"); session_runner._normalize_provider_identity() - # handles comparing this against older/bare-name forms. - def _extract_provider_identity() -> str: - return get_effective_config_summary(config, bundle_name).provider_module - # Helper to save session after each turn async def _save_session(): context = session.coordinator.get("context") @@ -2911,6 +2889,10 @@ 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 {} + # Resolve provider and model together so metadata always records a + # coherent pair. The resume check uses this same summary and expects + # its bare model value rather than a provider-qualified display label. + config_summary = get_effective_config_summary(config, bundle_name) metadata = { **existing_metadata, # Preserve name, description, etc. "session_id": actual_session_id, @@ -2918,8 +2900,8 @@ async def _save_session(): "created", datetime.now(UTC).isoformat() ), "bundle": bundle_name, - "model": _extract_model_name(), - "provider": _extract_provider_identity(), + "model": config_summary.model, + "provider": config_summary.provider_module, "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()), @@ -3659,6 +3641,10 @@ 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 {} + # Resolve provider and model together so metadata always records a + # coherent pair. Keep model_name above as the provider-qualified + # output label, but persist the bare model expected by resume. + config_summary = get_effective_config_summary(config, bundle_name) metadata = { **existing_metadata, # Preserve name, description, etc. "session_id": actual_session_id, @@ -3666,13 +3652,8 @@ def _goal_sigint_handler(signum, frame): "created", datetime.now(UTC).isoformat() ), "bundle": bundle_name, - "model": model_name, - # Same source (get_effective_config_summary) as the resume-time - # mismatch check in session_runner.py -- see _extract_provider_identity() - # in interactive_chat() above for the parallel single-shot-vs-chat note. - "provider": get_effective_config_summary( - config, bundle_name - ).provider_module, + "model": config_summary.model, + "provider": config_summary.provider_module, "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/tests/test_resume_provider_metadata_write.py b/tests/test_resume_provider_metadata_write.py index d1af5be3..c4f651c7 100644 --- a/tests/test_resume_provider_metadata_write.py +++ b/tests/test_resume_provider_metadata_write.py @@ -8,15 +8,15 @@ 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"` from `get_effective_config_summary()` -(`.provider_module`) -- the same function the read-side check uses to -compute the ACTIVE provider at resume -- so write-side and read-side always -agree on what "provider identity" means and how it's shaped (module id form, -e.g. "provider-anthropic"). +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 @@ -52,8 +52,10 @@ def _coordinator_get(key: str): 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 @@ -70,12 +72,20 @@ class TestExecuteSingleWritesProvider: """execute_single()'s final metadata-save block persists 'provider'.""" @pytest.mark.asyncio - async def test_metadata_includes_provider_matching_config(self, tmp_path: Path): - """The saved metadata's 'provider' matches the resolved config's provider_module.""" + 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() - session = _make_mock_session(hooks) + runtime_provider = MagicMock() + runtime_provider.model = "claude-x" + session = _make_mock_session(hooks, providers={"anthropic": runtime_provider}) initialized = _make_mock_initialized(session) config = { @@ -114,9 +124,35 @@ async def test_metadata_includes_provider_matching_config(self, tmp_path: Path): store_instance.save.assert_called_once() _saved_id, _saved_messages, saved_metadata = store_instance.save.call_args[0] - assert saved_metadata.get("provider") == "provider-anthropic" - # Sanity: 'model' is still written too (pre-existing field, untouched by this PR). - assert "model" in saved_metadata + 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( @@ -169,5 +205,97 @@ async def test_metadata_preserves_existing_fields_alongside_provider( _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}, + }, + ] + } + effective_summary = MagicMock() + effective_summary.provider_module = "provider-anthropic" + effective_summary.model = "claude-effective" + + 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}.get_effective_config_summary", + return_value=effective_summary, + ) as mock_summary, + 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" + mock_summary.assert_called_once_with(config, "bundle:test") From ca70a7733564dee1e0215161a206c25b8a78b9d7 Mon Sep 17 00:00:00 2001 From: Sam Schillace Date: Thu, 6 Aug 2026 16:22:04 -0700 Subject: [PATCH 3/4] fix: centralize session provider model provenance --- amplifier_app_cli/effective_config.py | 65 +++++++++--- amplifier_app_cli/incremental_save.py | 22 +--- amplifier_app_cli/main.py | 21 ++-- amplifier_app_cli/session_runner.py | 30 ++++-- amplifier_app_cli/session_spawner.py | 4 + tests/test_resume_provider_metadata_write.py | 105 +++++++++++++++++-- tests/test_session_runner.py | 67 ++++++++---- 7 files changed, 228 insertions(+), 86 deletions(-) diff --git a/amplifier_app_cli/effective_config.py b/amplifier_app_cli/effective_config.py index b9737978..0ee13e1b 100644 --- a/amplifier_app_cli/effective_config.py +++ b/amplifier_app_cli/effective_config.py @@ -39,6 +39,48 @@ 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") or 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 +94,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 +192,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 0cf51f0c..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 @@ -2889,10 +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 {} - # Resolve provider and model together so metadata always records a - # coherent pair. The resume check uses this same summary and expects - # its bare model value rather than a provider-qualified display label. - config_summary = get_effective_config_summary(config, bundle_name) + provenance = get_effective_provider_model(config) metadata = { **existing_metadata, # Preserve name, description, etc. "session_id": actual_session_id, @@ -2900,8 +2900,7 @@ async def _save_session(): "created", datetime.now(UTC).isoformat() ), "bundle": bundle_name, - "model": config_summary.model, - "provider": config_summary.provider_module, + **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()), @@ -3641,10 +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 {} - # Resolve provider and model together so metadata always records a - # coherent pair. Keep model_name above as the provider-qualified - # output label, but persist the bare model expected by resume. - config_summary = get_effective_config_summary(config, bundle_name) + provenance = get_effective_provider_model(config) metadata = { **existing_metadata, # Preserve name, description, etc. "session_id": actual_session_id, @@ -3652,8 +3648,7 @@ def _goal_sigint_handler(signum, frame): "created", datetime.now(UTC).isoformat() ), "bundle": bundle_name, - "model": config_summary.model, - "provider": config_summary.provider_module, + **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 86e60413..288964ac 100644 --- a/amplifier_app_cli/session_runner.py +++ b/amplifier_app_cli/session_runner.py @@ -43,7 +43,7 @@ from amplifier_core import AmplifierSession from amplifier_core import ModuleValidationError -from .effective_config import get_effective_config_summary +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 @@ -363,7 +363,7 @@ async def create_initialized_session( # its metadata. -def _normalize_provider_identity(value: str | None) -> str: +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") @@ -384,12 +384,9 @@ def _normalize_provider_identity(value: str | None) -> str: Lowercased bare provider name (e.g. "anthropic"), or "" if value is falsy. """ - if not value: + if not isinstance(value, str) or not value: return "" - normalized = value.strip().lower() - if normalized.startswith("provider-"): - normalized = normalized[len("provider-") :] - return normalized + return value.strip().lower().removeprefix("provider-") async def _warn_on_resume_provider_mismatch( @@ -458,6 +455,19 @@ async def _warn_on_resume_provider_mismatch( ) 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") @@ -467,9 +477,9 @@ async def _warn_on_resume_provider_mismatch( if not prior_model and not prior_provider: return - summary = get_effective_config_summary(config.config, config.bundle_name) - active_provider = summary.provider_module - active_model = summary.model + 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. 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/tests/test_resume_provider_metadata_write.py b/tests/test_resume_provider_metadata_write.py index c4f651c7..5f1852db 100644 --- a/tests/test_resume_provider_metadata_write.py +++ b/tests/test_resume_provider_metadata_write.py @@ -92,7 +92,11 @@ async def test_metadata_writes_bare_effective_pair_and_resumes_silently( "providers": [ { "module": "provider-anthropic", - "config": {"default_model": "claude-x", "priority": 0}, + "config": { + "model": "claude-x", + "default_model": "stale-default", + "priority": 0, + }, } ] } @@ -243,10 +247,6 @@ async def test_metadata_uses_effective_pair_not_first_provider( }, ] } - effective_summary = MagicMock() - effective_summary.provider_module = "provider-anthropic" - effective_summary.model = "claude-effective" - with ( patch( f"{_MODULE}.create_initialized_session", @@ -254,10 +254,6 @@ async def test_metadata_uses_effective_pair_not_first_provider( ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch( - f"{_MODULE}.get_effective_config_summary", - return_value=effective_summary, - ) as mock_summary, patch(f"{_MODULE}._create_prompt_session", return_value=prompt_session), patch( f"{_MODULE}.patch_stdout", @@ -298,4 +294,93 @@ async def test_metadata_uses_effective_pair_not_first_provider( assert saved_metadata["provider"] == "provider-anthropic" assert saved_metadata["model"] == "claude-effective" assert saved_metadata["name"] == "keep-me" - mock_summary.assert_called_once_with(config, "bundle:test") + + +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 c3b945c7..0432930b 100644 --- a/tests/test_session_runner.py +++ b/tests/test_session_runner.py @@ -858,7 +858,7 @@ async def test_configurator_none_on_general_exception(self, caplog): import click as _click # noqa: E402 (grouped here to stay close to its tests) -from amplifier_app_cli.effective_config import EffectiveConfigSummary # noqa: E402 +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, @@ -867,17 +867,9 @@ async def test_configurator_none_on_general_exception(self, caplog): def _make_summary( provider_module: str = "provider-anthropic", model: str = "claude-x" -) -> EffectiveConfigSummary: - """Return a minimal EffectiveConfigSummary for mismatch-check tests.""" - return EffectiveConfigSummary( - config_source="bundle:test", - provider_name=provider_module.replace("provider-", "").title(), - provider_module=provider_module, - model=model, - orchestrator="loop-basic", - tool_count=0, - hook_count=0, - ) +) -> EffectiveProviderModel: + """Return canonical provenance for mismatch-check tests.""" + return EffectiveProviderModel(provider=provider_module, model=model) class TestNormalizeProviderIdentity: @@ -918,7 +910,7 @@ async def test_mismatch_tty_prints_warning_and_confirms_then_proceeds(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="provider-openai", model="gpt-x" ), @@ -950,7 +942,7 @@ async def test_mismatch_tty_decline_aborts_cleanly(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="provider-openai", model="gpt-x" ), @@ -978,7 +970,7 @@ async def test_mismatch_non_tty_warns_and_continues_without_confirm(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="provider-openai", model="gpt-x" ), @@ -1006,7 +998,7 @@ async def test_provider_and_model_match_is_completely_silent(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="provider-anthropic", model="claude-x" ), @@ -1033,7 +1025,7 @@ async def test_missing_provider_field_model_differs_falls_back_to_model_only(sel with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="provider-openai", model="gpt-x" ), @@ -1058,7 +1050,7 @@ async def test_missing_provider_field_model_matches_is_silent(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="provider-anthropic", model="claude-x" ), @@ -1082,7 +1074,7 @@ async def test_provider_normalization_prevents_false_positive(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary( provider_module="anthropic", model="claude-x" ), @@ -1109,7 +1101,7 @@ async def test_no_prior_metadata_at_all_is_silent(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", return_value=_make_summary() + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary() ), patch(f"{_MODULE}.click.confirm") as mock_confirm, ): @@ -1129,7 +1121,7 @@ async def test_prior_metadata_load_failure_is_silent_best_effort(self): with ( patch(f"{_MODULE}.SessionStore") as MockStore, patch( - f"{_MODULE}.get_effective_config_summary", return_value=_make_summary() + f"{_MODULE}.get_effective_provider_model", return_value=_make_summary() ), ): MockStore.return_value.get_metadata.side_effect = FileNotFoundError("nope") @@ -1137,6 +1129,39 @@ async def test_prior_metadata_load_failure_is_silent_best_effort(self): 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 TestProviderMismatchCheckWiredIntoChokepoint: """Integration: create_initialized_session gates the check on config.is_resume. From 4c03dd1d9c3f73cfded95acb009edf3b05084880 Mon Sep 17 00:00:00 2001 From: Sam Schillace Date: Thu, 6 Aug 2026 16:41:25 -0700 Subject: [PATCH 4/4] fix(cli): harden delegated resume metadata --- amplifier_app_cli/commands/session.py | 3 +- amplifier_app_cli/effective_config.py | 4 +- amplifier_app_cli/session_store.py | 26 +++- tests/test_session_runner.py | 64 ++++++++ tests/test_session_spawner.py | 186 +++++++++++++++++++++++ tests/test_session_store_sanitization.py | 26 +++- 6 files changed, 298 insertions(+), 11 deletions(-) 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 0ee13e1b..5f465557 100644 --- a/amplifier_app_cli/effective_config.py +++ b/amplifier_app_cli/effective_config.py @@ -74,7 +74,9 @@ def get_effective_provider_model(config: dict[str, Any]) -> EffectiveProviderMod if not isinstance(provider_config, dict): provider_config = {} - model = provider_config.get("model") or provider_config.get("default_model") + 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" 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_session_runner.py b/tests/test_session_runner.py index 0432930b..91c3d9c6 100644 --- a/tests/test_session_runner.py +++ b/tests/test_session_runner.py @@ -1163,6 +1163,70 @@ async def test_parseable_malformed_metadata_is_silently_ignored(self, metadata): 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. 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: