From ed3f75c567546cef21148613183410974ca519d7 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Mon, 3 Aug 2026 05:58:44 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20green=20the=20test=20suite=20=E2=80=94?= =?UTF-8?q?=202=20real=20product=20bugs,=2013=20stale=20tests,=20no=20CI?= =?UTF-8?q?=20exclusions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- .github/workflows/ci.yml | 71 ++++---- amplifier_app_cli/commands/provider.py | 15 +- amplifier_app_cli/provider_config_utils.py | 56 +++--- amplifier_app_cli/session_spawner.py | 11 +- tests/test_always_render_final_response.py | 4 +- tests/test_cleanup_observability.py | 12 +- tests/test_handler_methods.py | 24 ++- tests/test_provider_commands.py | 8 +- tests/test_provider_instance_credentials.py | 186 ++++++++++++++++++++ tests/test_session_lifecycle_events.py | 6 +- tests/test_session_spawner_issue_233.py | 56 ++++++ tests/test_session_spawner_subprocess.py | 55 +++++- 12 files changed, 424 insertions(+), 80 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97331154..40a627e9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,43 +28,34 @@ jobs: run: uv sync --all-extras --dev - name: Run test suite - run: | - # 15 pre-existing failures on `main`, unrelated to this workflow and - # to amplifier_app_cli/dedicated_tty_input.py -- confirmed identical - # with and without the tty-pollability-probe change via a local - # `git stash` A/B comparison (15 failed, N passed on both sides). - # They are excluded here BY NODE ID (not by file, so every other - # test in these files still runs in CI) rather than papered over - # with `|| true`, so a *new* failure in any of these files still - # fails the build. Root causes (unrelated to TTY input handling): - # - tests/test_always_render_final_response.py: overlay/streaming - # config assertions out of sync with current render behavior. - # - tests/test_cleanup_observability.py: cleanup event ordering - # assertions out of sync with current event emission. - # - tests/test_handler_methods.py: skill-prompt arg formatting - # assertions out of sync with current formatting. - # - tests/test_provider_commands.py: provider priority assertion - # out of sync with current provider-add behavior. - # - tests/test_session_lifecycle_events.py: session_end event - # count/payload assertions out of sync with current emission. - # - tests/test_session_spawner_subprocess.py: spawned subprocess - # config now includes an `agents` key the test doesn't expect. - # TODO(#): investigate and fix (or update) these - # pre-existing failures; they are out of scope for the TTY - # pollability probe fix and were not introduced by it. - uv run pytest -q \ - --deselect "tests/test_always_render_final_response.py::TestAlwaysRenderFinalResponse::test_render_message_called_even_when_overlay_would_be_active" \ - --deselect "tests/test_always_render_final_response.py::TestAlwaysRenderFinalResponse::test_render_message_called_when_no_streaming_config" \ - --deselect "tests/test_cleanup_observability.py::TestExecuteSingleCleanupEvents::test_cleanup_events_emitted_in_order" \ - --deselect "tests/test_cleanup_observability.py::TestExecuteSingleCleanupEvents::test_cleanup_render_begin_before_store_begin" \ - --deselect "tests/test_cleanup_observability.py::TestExecuteSingleCleanupEvents::test_store_end_payload_has_message_count" \ - --deselect "tests/test_cleanup_observability.py::TestExecuteSingleCleanupEvents::test_all_cleanup_events_carry_session_id" \ - --deselect "tests/test_cleanup_observability.py::TestExecuteSingleCleanupEvents::test_cleanup_events_emitted_in_json_mode" \ - --deselect "tests/test_cleanup_observability.py::TestExecuteSingleNoHooks::test_runs_without_hooks" \ - --deselect "tests/test_handler_methods.py::TestLoadSkillPromptWithArgs::test_load_skill_prompt_with_args_exact_format" \ - --deselect "tests/test_handler_methods.py::TestLoadSkillPromptWithArgs::test_load_skill_with_different_args" \ - --deselect "tests/test_provider_commands.py::TestProviderAdd::test_provider_add_assigns_priority" \ - --deselect "tests/test_session_lifecycle_events.py::TestSessionEndExactlyOnce::test_execute_single_emits_session_end_exactly_once" \ - --deselect "tests/test_session_lifecycle_events.py::TestSessionEndExactlyOnce::test_execute_single_session_end_emitted_not_zero_times" \ - --deselect "tests/test_session_lifecycle_events.py::TestSessionEndExactlyOnce::test_execute_single_session_end_payload_has_session_id" \ - --deselect "tests/test_session_spawner_subprocess.py::TestSubprocessRouting::test_subprocess_param_routes_to_subprocess" + # No exclusions. The whole suite runs on every push and PR, on both + # platforms. If you are tempted to add a --deselect here, fix the test + # or fix the product instead -- an excluded test is a test nobody is + # watching, which is how a total macOS breakage (#247) shipped. + run: uv run pytest -q + + integration: + name: pytest -m integration (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest] + steps: + - uses: actions/checkout@v4 + + - name: Install uv + uses: astral-sh/setup-uv@v5 + with: + python-version: "3.12" + + - name: Install dependencies + run: uv sync --all-extras --dev + + - name: Run integration tests + # pyproject.toml sets `addopts = -m "not integration"`, so these tests + # -- the ones that fork a real pty child and probe real termios state + # -- are skipped by default and, before this job existed, ran nowhere. + # They are exactly the tests that guard the dedicated-tty-input + # mechanism, so they get their own job on both platforms. + run: uv run pytest -m integration -q diff --git a/amplifier_app_cli/commands/provider.py b/amplifier_app_cli/commands/provider.py index 00d44bed..c3269d90 100644 --- a/amplifier_app_cli/commands/provider.py +++ b/amplifier_app_cli/commands/provider.py @@ -16,7 +16,7 @@ from ..lib.settings import AppSettings, Scope from ..paths import create_config_manager from ..provider_config_utils import ( - _claimed_env_vars, + _config_claimed_env_vars, _normalize_id, _secret_env_var_for, _secret_field_id_for, @@ -237,8 +237,19 @@ def _resolve_env_var_overrides( ValueError if the instance id cannot produce a usable, non-colliding suggestion (design §5.4.2) -- caller decides how to react (re-prompt vs. exit). + + Collision detection asks "does another *configured instance* already own + this name?", so it uses ``_config_claimed_env_vars`` (${VAR} references + in some scope's provider config) and NOT the write-side + ``_claimed_env_vars``, which also counts every name sitting in + ``keys.env``. A keys.env-only leftover -- e.g. the key a previously + removed instance left behind, since remove deliberately doesn't delete + it (§8 risk 6) -- is owned by nobody: the first instance of that type + must keep today's no-extra-prompt UX (§5.2 step 3) and land on the + stale-credential warn-and-reuse path (§5.4.4), not be pushed into a + collision rename against an instance that doesn't exist. """ - claimed = _claimed_env_vars(settings) + claimed = _config_claimed_env_vars(settings) default_name = _secret_env_var_for(module_id) if not default_name or default_name not in claimed: return {} diff --git a/amplifier_app_cli/provider_config_utils.py b/amplifier_app_cli/provider_config_utils.py index 7c5137d7..400703d3 100644 --- a/amplifier_app_cli/provider_config_utils.py +++ b/amplifier_app_cli/provider_config_utils.py @@ -266,11 +266,18 @@ def _secret_field_id_for(module_id: str) -> str | None: return field.get("id") if field else None -def _claimed_env_vars(settings: AppSettings) -> set[str]: - """Env-var names already spoken for, by ANY means, across ALL scopes - (global, project, local, session): either referenced by a ``${VAR}`` - placeholder in some scope's provider config, OR already backed by a - real, saved secret in ``~/.amplifier/keys.env``. +def _config_claimed_env_vars(settings: AppSettings) -> set[str]: + """Env-var names claimed by an existing *configured instance*: names + referenced by a ``${VAR}`` placeholder in some scope's provider config + (global, project, local, session). + + This is the design's notion of "claimed" for the add paths' + collision detection (§5.2 steps 3-4): a name is spoken for when a + provider entry actually points at it. A name that exists only as a + leftover secret in ``keys.env`` -- with no entry referencing it -- is + NOT claimed here: no instance owns it, so a first instance of that + type may legitimately (re)use it. That case is the stale-credential + warn-and-reuse path (§5.4.4), not a collision. Mirrors ``AppSettings.get_provider_overrides()``'s scope iteration order, but deliberately does NOT mirror its silent @@ -279,15 +286,7 @@ def _claimed_env_vars(settings: AppSettings) -> set[str]: would let a new instance claim an already-used env var and reintroduce Bug 3 through a different door. - A literal (non-placeholder) config value claims nothing BY ITSELF -- - it's the presence of an actual saved key in keys.env (checked below, - once per call) that claims a name, not the shape of the config value - referencing it. This matters for the race where one instance's literal - secret has just been normalized and saved to keys.env, but another - entry's still-unprocessed literal in the same write batch hasn't been - touched yet: without this, the second entry's default name would look - unclaimed and clobber the first instance's freshly-saved secret in - keys.env. See docs/designs/provider-instance-credentials.md §5.4.1. + A literal (non-placeholder) config value claims nothing. """ claimed: set[str] = set() for scope in ("global", "project", "local", "session"): @@ -319,16 +318,31 @@ def _claimed_env_vars(settings: AppSettings) -> set[str]: if isinstance(v, str) and v.startswith("${") and v.endswith("}"): claimed.add(v[2:-1]) - # Also claim any name already backed by a real, saved secret in - # keys.env, even if no scope's config currently references it via a - # placeholder yet (e.g. it was just saved moments ago by another - # entry's normalization/configure_provider call within the same - # command, before this scope's write has landed). Single read, reused - # by the caller's loop -- not re-read per provider entry. - claimed |= KeyManager().stored_keys() return claimed +def _claimed_env_vars(settings: AppSettings) -> set[str]: + """Config claims (see ``_config_claimed_env_vars``) PLUS every name + already backed by a real, saved secret in ``~/.amplifier/keys.env``. + + This is the WRITE-side notion, used where reusing a name would + *overwrite an actual stored secret*: a name already backed by a saved + key is spoken for even when no scope's config references it via a + placeholder yet (e.g. it was saved moments ago by another entry's + normalization/configure_provider call within the same command, before + this scope's write has landed). Without it, the second entry's default + name would look unclaimed and clobber the first instance's + freshly-saved secret in keys.env. Single read, reused by the caller's + loop -- not re-read per provider entry. + See docs/designs/provider-instance-credentials.md §5.4.1. + + Do NOT use this for the add paths' collision detection: a keys.env-only + leftover means no instance owns the name (§5.4.4 warn-and-reuse), not + that a collision exists -- use ``_config_claimed_env_vars`` there. + """ + return _config_claimed_env_vars(settings) | KeyManager().stored_keys() + + def _suggest_instance_env_var( module_id: str, instance_id: str, claimed: set[str] ) -> str: diff --git a/amplifier_app_cli/session_spawner.py b/amplifier_app_cli/session_spawner.py index ff7e3703..ac590efd 100644 --- a/amplifier_app_cli/session_spawner.py +++ b/amplifier_app_cli/session_spawner.py @@ -320,10 +320,19 @@ async def spawn_sub_session( except AttributeError: live_agents = {} if live_agents: - child_agents = merged_config.setdefault("agents", {}) + # Build a FRESH dict and rebind it; never mutate the dict + # merged_config already holds. merge_configs() deep-copies the + # agents dict only when it is non-empty, and merge_agent_dicts() + # starts from a shallow parent.copy() -- so an EMPTY parent + # "agents" dict arrives here as the parent session's own object. + # setdefault()-then-mutate would then write the live registry + # straight into the parent's live config and hand the child the + # very same dict (cross-session state leak). + child_agents = dict(merged_config.get("agents") or {}) for name, cfg in live_agents.items(): if name not in child_agents: child_agents[name] = copy.deepcopy(cfg) + merged_config["agents"] = child_agents # === end issue #233 fix (agents) === # Apply tool inheritance filtering if specified diff --git a/tests/test_always_render_final_response.py b/tests/test_always_render_final_response.py index 8e6a713c..717e101a 100644 --- a/tests/test_always_render_final_response.py +++ b/tests/test_always_render_final_response.py @@ -117,7 +117,7 @@ async def test_render_message_called_even_when_overlay_would_be_active( patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), patch( - f"{_MODULE}._process_runtime_mentions", + f"{_MODULE}.process_runtime_mentions", new=AsyncMock(side_effect=lambda s, t: t), ), patch(f"{_MODULE}.get_effective_config_summary"), @@ -168,7 +168,7 @@ async def test_render_message_called_when_no_streaming_config( patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), patch( - f"{_MODULE}._process_runtime_mentions", + f"{_MODULE}.process_runtime_mentions", new=AsyncMock(side_effect=lambda s, t: t), ), patch(f"{_MODULE}.get_effective_config_summary"), diff --git a/tests/test_cleanup_observability.py b/tests/test_cleanup_observability.py index bbb21d38..e3b93259 100644 --- a/tests/test_cleanup_observability.py +++ b/tests/test_cleanup_observability.py @@ -210,7 +210,7 @@ async def test_cleanup_events_emitted_in_order(self, tmp_path: Path): ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), # suppress Rich output - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): # Minimal SessionStore mock store_instance = MockStore.return_value @@ -273,7 +273,7 @@ async def test_cleanup_render_begin_before_store_begin(self, tmp_path: Path): ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} @@ -315,7 +315,7 @@ async def test_store_end_payload_has_message_count(self, tmp_path: Path): ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} @@ -354,7 +354,7 @@ async def test_all_cleanup_events_carry_session_id(self, tmp_path: Path): ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} @@ -398,7 +398,7 @@ async def test_cleanup_events_emitted_in_json_mode(self, tmp_path: Path): ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} @@ -475,7 +475,7 @@ def _coordinator_get_nohooks(key: str): ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} diff --git a/tests/test_handler_methods.py b/tests/test_handler_methods.py index e6e96e4e..838538dc 100644 --- a/tests/test_handler_methods.py +++ b/tests/test_handler_methods.py @@ -395,8 +395,20 @@ async def test_load_skill_prompt_with_args_exact_format(self): cp = _make_command_processor(skills_discovery=mock_discovery) is_prompt, text = await cp._load_skill("simplify", "focus on memory usage") assert is_prompt is True - # Exact format: 'Use the load_skill tool to load the skill "". Additional context from the user: ' - expected = 'Use the load_skill tool to load the skill "simplify". Additional context from the user: focus on memory usage' + # Exact format (per commit 1e6bed1 "forward /command arguments into + # load_skill so fork skills receive them"): the synthetic prompt must + # explicitly instruct the model to forward the user's text as the + # `arguments` parameter of the load_skill tool call -- a plain + # "Additional context" mention is not enough because a forked + # sub-session cannot see this parent conversation and can only + # receive $ARGUMENTS via that parameter. + expected = ( + 'Use the load_skill tool to load the skill "simplify", ' + "passing the user's input as the `arguments` parameter " + '(load_skill(skill_name="simplify", arguments=...)) so the skill ' + "receives it — this is required for fork skills, which cannot otherwise " + "see it. The user's input is: focus on memory usage" + ) assert text == expected @pytest.mark.asyncio @@ -411,7 +423,13 @@ async def test_load_skill_with_different_args(self): cp = _make_command_processor(skills_discovery=mock_discovery) is_prompt, text = await cp._load_skill("refactor", "please clean this up") assert is_prompt is True - expected = 'Use the load_skill tool to load the skill "refactor". Additional context from the user: please clean this up' + expected = ( + 'Use the load_skill tool to load the skill "refactor", ' + "passing the user's input as the `arguments` parameter " + '(load_skill(skill_name="refactor", arguments=...)) so the skill ' + "receives it — this is required for fork skills, which cannot otherwise " + "see it. The user's input is: please clean this up" + ) assert text == expected @pytest.mark.asyncio diff --git a/tests/test_provider_commands.py b/tests/test_provider_commands.py index 719247db..925ed784 100644 --- a/tests/test_provider_commands.py +++ b/tests/test_provider_commands.py @@ -109,8 +109,14 @@ def test_provider_add_saves_entry_to_settings(self, tmp_path, monkeypatch): assert added["module"] == "provider-anthropic" assert added["config"]["default_model"] == "claude-sonnet-4-6" - def test_provider_add_assigns_priority(self, tmp_path): + def test_provider_add_assigns_priority(self, tmp_path, monkeypatch): """First provider gets priority 1, subsequent get max+1.""" + # Isolate HOME like the sibling tests do: the add path consults + # ~/.amplifier/keys.env (KeyManager) while resolving this instance's + # credential env var, so without this the test reads the developer's + # real key store and its result depends on which keys they happen to + # have saved. + monkeypatch.setattr(Path, "home", lambda: tmp_path) settings = _make_settings(tmp_path) # Seed an existing provider with priority 1 diff --git a/tests/test_provider_instance_credentials.py b/tests/test_provider_instance_credentials.py index e2b4f4ab..ff02ef5f 100644 --- a/tests/test_provider_instance_credentials.py +++ b/tests/test_provider_instance_credentials.py @@ -994,6 +994,192 @@ def test_warns_and_reuses_keys_env_only_leftover(self, tmp_path, monkeypatch): assert "stored credential" in printed.lower() assert "reused" in printed.lower() + def test_real_key_manager_leftover_is_reused_not_rejected( + self, tmp_path, monkeypatch + ): + """The §5.4.4 warn-and-reuse path must be reachable with a REAL + KeyManager, not only with a mocked one. + + Regression: collision detection used the write-side + ``_claimed_env_vars``, which counts every name sitting in keys.env. + A keys.env leftover was therefore rejected before the user could + accept it -- ``_suggest_instance_env_var`` raised "already in use by + another instance" for the very name §5.4.4 says to reuse, making + this branch dead in production (it only passed above because the + mocked key manager and the empty real keys.env disagreed). + """ + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import _resolve_env_var_overrides + from amplifier_app_cli.key_manager import KeyManager + + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"api_key": "${ANTHROPIC_API_KEY}"}, + provider_id="anthropic-opus", + scope="global", + ) + # keys.env-only leftover from a previously-removed 'anthropic-fable' + # (remove deliberately does not delete the key -- §8 risk 6). + # setenv first so pytest restores the process env at teardown; + # save_key() writes os.environ itself. + monkeypatch.setenv("ANTHROPIC_FABLE_API_KEY", "sk-stale-fable") + KeyManager().save_key("ANTHROPIC_FABLE_API_KEY", "sk-stale-fable") + key_manager = KeyManager() + + with ( + patch( + "amplifier_app_cli.commands.provider._secret_env_var_for", + return_value="ANTHROPIC_API_KEY", + ), + patch( + "amplifier_app_cli.commands.provider.Prompt.ask", + return_value="ANTHROPIC_FABLE_API_KEY", + ), + patch("amplifier_app_cli.commands.provider.console") as mock_console, + ): + overrides = _resolve_env_var_overrides( + settings, key_manager, "provider-anthropic", "anthropic-fable" + ) + + assert overrides == {"ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY"} + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "stored credential" in printed.lower() + assert "already in use by another" not in printed.lower() + + +# ============================================================ +# A keys.env-only leftover is NOT a collision (§5.2 step 3) +# ============================================================ + + +class TestStaleKeysEnvIsNotACollision: + """The type default being present in keys.env, with no provider entry + referencing it, must not push the FIRST instance of that type onto the + collision path -- nobody owns that name.""" + + def test_first_instance_gets_no_collision_prompt(self, tmp_path, monkeypatch): + """No configured instance references ANTHROPIC_API_KEY, so adding the + first anthropic instance keeps today's UX: default name, no prompt.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import _resolve_env_var_overrides + from amplifier_app_cli.key_manager import KeyManager + + settings = _make_settings(tmp_path) + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-left-over") + KeyManager().save_key("ANTHROPIC_API_KEY", "sk-left-over") + + with ( + patch( + "amplifier_app_cli.commands.provider._secret_env_var_for", + return_value="ANTHROPIC_API_KEY", + ), + patch("amplifier_app_cli.commands.provider.Prompt.ask") as mock_ask, + ): + overrides = _resolve_env_var_overrides( + settings, MagicMock(), "provider-anthropic", None + ) + + assert overrides == {}, ( + "A keys.env-only leftover is owned by no instance -- the first " + "instance of the type must use the type default (§5.2 step 3)." + ) + mock_ask.assert_not_called() + + def test_second_instance_still_collides(self, tmp_path, monkeypatch): + """Guard the other direction: a name a configured instance actually + references still triggers the collision path.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import _resolve_env_var_overrides + + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"api_key": "${ANTHROPIC_API_KEY}"}, + provider_id="anthropic-opus", + scope="global", + ) + mock_key_manager = MagicMock() + mock_key_manager.has_key.return_value = False + mock_key_manager.has_stored_key.return_value = False + + with ( + patch( + "amplifier_app_cli.commands.provider._secret_env_var_for", + return_value="ANTHROPIC_API_KEY", + ), + patch( + "amplifier_app_cli.commands.provider.Prompt.ask", + return_value="ANTHROPIC_FABLE_API_KEY", + ) as mock_ask, + ): + overrides = _resolve_env_var_overrides( + settings, mock_key_manager, "provider-anthropic", "anthropic-fable" + ) + + assert overrides == {"ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY"} + mock_ask.assert_called_once() + + def test_provider_add_cli_writes_entry_despite_stale_key( + self, tmp_path, monkeypatch + ): + """`provider add ` must still add the provider when keys.env + already holds the type's default credential name. + + User-visible regression this pins: with a leftover key (which + ``provider add`` itself wrote, and ``provider remove`` deliberately + keeps), the add path announced a collision against "an existing + instance" that does not exist, prompted for a per-instance env var, + and -- with nothing on stdin -- printed "Cancelled." and exited 0 + having written no provider at all. + """ + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import provider + from amplifier_app_cli.key_manager import KeyManager + + settings = _make_settings(tmp_path) + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-left-over") + KeyManager().save_key("ANTHROPIC_API_KEY", "sk-left-over") + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch("amplifier_app_cli.commands.provider._ensure_providers_ready"), + patch( + "amplifier_app_cli.commands.provider.configure_provider", + return_value={ + "default_model": "claude-sonnet-4-6", + "api_key": "${ANTHROPIC_API_KEY}", + }, + ), + patch("amplifier_app_cli.commands.provider.KeyManager"), + patch("amplifier_app_cli.commands.provider.ProviderManager") as MockPM, + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_mock_provider_info(), + ), + ): + mock_pm = MagicMock() + mock_pm.list_providers.return_value = [ + ("provider-anthropic", "Anthropic", "Anthropic provider"), + ] + MockPM.return_value = mock_pm + + result = runner.invoke(provider, ["add", "anthropic"]) + + assert result.exit_code == 0, f"Output: {result.output}" + assert "Cancelled" not in result.output, ( + f"add path cancelled instead of adding the provider: {result.output}" + ) + providers = settings.get_scope_provider_overrides("global") + assert [p["module"] for p in providers] == ["provider-anthropic"] + assert providers[0]["config"]["api_key"] == "${ANTHROPIC_API_KEY}" + # ============================================================ # Non-interactive fail-loud (§5.4.5) diff --git a/tests/test_session_lifecycle_events.py b/tests/test_session_lifecycle_events.py index 6cd794b5..4ce17190 100644 --- a/tests/test_session_lifecycle_events.py +++ b/tests/test_session_lifecycle_events.py @@ -142,7 +142,7 @@ async def test_execute_single_emits_session_end_exactly_once(self, tmp_path: Pat ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} @@ -182,7 +182,7 @@ async def test_execute_single_session_end_emitted_not_zero_times( ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} @@ -220,7 +220,7 @@ async def test_execute_single_session_end_payload_has_session_id( ), patch(f"{_MODULE}.SessionStore") as MockStore, patch(f"{_MODULE}.console"), - patch(f"{_MODULE}._process_runtime_mentions", new=AsyncMock()), + patch(f"{_MODULE}.process_runtime_mentions", new=AsyncMock()), ): store_instance = MockStore.return_value store_instance.get_metadata.return_value = {} diff --git a/tests/test_session_spawner_issue_233.py b/tests/test_session_spawner_issue_233.py index 26c9eb76..b3f72530 100644 --- a/tests/test_session_spawner_issue_233.py +++ b/tests/test_session_spawner_issue_233.py @@ -423,3 +423,59 @@ async def test_skill_capability_propagated_to_child_coordinator(self) -> None: f"must contain the same skills as the parent. " f"Expected {skills_list!r}, got {registered!r}" ) + + +# --------------------------------------------------------------------------- +# S6 — Isolation: propagation must not reach back into the parent +# --------------------------------------------------------------------------- + + +class TestS6ParentIsolation: + """The propagation writes into the CHILD's config only, never the parent's.""" + + @pytest.mark.asyncio + async def test_propagation_does_not_mutate_parent_config(self) -> None: + """An empty `agents` dict in session.config must not become the + parent's copy of the live registry. + + merge_configs() deep-copies the merged `agents` dict only when it is + non-empty, and merge_agent_dicts() starts from a shallow parent.copy() + — so when the parent's session.config carries an EMPTY agents dict + (e.g. it was itself spawned with `agents: none`), the merged config's + "agents" value IS the parent's own dict object. Filling that dict in + place would silently rewrite the running parent session's config and + hand the child the same object, so a later mutation on either side is + visible to the other. + """ + parent = _make_parent_session( + session_config_agents={}, # present but empty — the aliasing trigger + coordinator_config_agents={"mode_agent_A": {"description": "Agent A"}}, + ) + parent_agents_before = parent.config["agents"] + child = _make_child_session_mock() + captured: dict = {} + + await _run_spawn( + parent, + {"mode_agent_A": {"description": "Agent A"}}, + child, + "mode_agent_A", + captured, + ) + + # Child sees the live registry (issue #233 behavior, unchanged). + assert "mode_agent_A" in captured.get("agents", {}) + + # ...and the parent's own config is untouched. + assert parent.config["agents"] == {}, ( + "spawn_sub_session mutated the PARENT session's config['agents'] " + f"in place: {parent.config['agents']!r}" + ) + assert parent_agents_before == {} + assert captured["agents"] is not parent.config["agents"], ( + "child and parent share the same agents dict object — a later " + "mutation on either side would leak across the session boundary" + ) + assert captured["agents"] is not parent.coordinator.config["agents"], ( + "child's agents dict is the parent's LIVE registry object" + ) diff --git a/tests/test_session_spawner_subprocess.py b/tests/test_session_spawner_subprocess.py index 35445a26..cd69462a 100644 --- a/tests/test_session_spawner_subprocess.py +++ b/tests/test_session_spawner_subprocess.py @@ -24,7 +24,9 @@ def anyio_backend(): return "asyncio" -def _make_parent_session(config=None, session_id="parent-session-id"): +def _make_parent_session( + config=None, session_id="parent-session-id", coordinator_config=None +): """Create a minimal mock parent session for testing. Creates a mock with session_id, config, and coordinator attributes, @@ -38,6 +40,14 @@ def _make_parent_session(config=None, session_id="parent-session-id"): # Mock coordinator coordinator = MagicMock() + # A real coordinator's `config` is a plain dict (RustCoordinator.config -> + # dict[str, Any]); spawn_sub_session reads coordinator.config["agents"] as + # the LIVE agent registry (issue #233). Leaving it as an auto-created + # MagicMock attribute would make that lookup return a truthy Mock and + # fabricate an agents entry no real session would produce. + coordinator.config = ( + dict(parent.config) if coordinator_config is None else coordinator_config + ) coordinator.get_capability.return_value = None # Default: no capabilities coordinator.get.return_value = None # No mounted modules by default coordinator.approval_system = MagicMock() @@ -106,6 +116,49 @@ async def test_subprocess_param_routes_to_subprocess(self, monkeypatch): assert result["turn_count"] == 1 assert result["metadata"] == {} + async def test_live_registry_agents_propagate_to_subprocess_config( + self, monkeypatch + ): + """Live coordinator.config['agents'] reaches the subprocess child. + + The issue #233 propagation applies to the subprocess path too: the + runtime agent registry lives in coordinator.config, not in the static + session.config snapshot, so it must be merged into the config handed + to run_session_in_subprocess. Pins that the `agents` key appears only + when there is something to propagate (see + test_subprocess_param_routes_to_subprocess, which asserts the exact + config for a parent with an empty live registry). + """ + parent = _make_parent_session( + coordinator_config={"agents": {"mode_sibling": {"description": "B"}}} + ) + + fake_module = _make_subprocess_runner_module() + monkeypatch.setitem( + sys.modules, "amplifier_foundation.subprocess_runner", fake_module + ) + + with ( + patch("amplifier_app_cli.session_spawner.merge_configs") as mock_merge, + ): + mock_merge.return_value = {"session": {}} + + from amplifier_app_cli.session_spawner import spawn_sub_session + + await spawn_sub_session( + agent_name="some-agent", + instruction="Do something", + parent_session=parent, + agent_configs={"some-agent": {}}, + sub_session_id="fixed-test-id", + use_subprocess=True, + ) + + passed_config = fake_module.run_session_in_subprocess.call_args.kwargs["config"] + assert passed_config["agents"] == {"mode_sibling": {"description": "B"}} + # The child must get its own copy -- never the parent's live registry. + assert passed_config["agents"] is not parent.coordinator.config["agents"] + async def test_spawn_mode_config_routes_to_subprocess(self, monkeypatch): """spawn_mode: subprocess in merged config routes to subprocess runner.