fix: green the test suite — 2 real product bugs, 13 stale tests, no CI exclusions

.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(#<tracking-issue>): 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

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 {}

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:

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

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"),

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 = {}

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 "<name>". Additional context from the user: <args>'
-        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

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