diff --git a/amplifier_foundation/subprocess_runner.py b/amplifier_foundation/subprocess_runner.py index 2f162381..730a4cb1 100644 --- a/amplifier_foundation/subprocess_runner.py +++ b/amplifier_foundation/subprocess_runner.py @@ -592,13 +592,62 @@ async def _run_child_session(config_path: str) -> str: logger.debug("Subprocess child session initialized, capabilities registered") - # (7) Expand @mentions in the prompt before execute. Mirrors PreparedBundle.spawn. + # Shared mention-expansion inputs, resolved once and reused below for both + # the agent's system instruction (persona) and the prompt (step 7). from amplifier_foundation.mentions import expand_mentions_in_instruction _mention_resolver = session.coordinator.get_capability("mention_resolver") _mention_dedup = session.coordinator.get_capability("mention_deduplicator") _working_dir = session.coordinator.get_capability("session.working_dir") _relative_to = Path(_working_dir) if _working_dir else None + + # (6b) Register the agent's system instruction (persona) on the child context. + # + # WHY THIS EXISTS: the `instruction` key crosses the process boundary intact + # inside `config` (merge_configs() on the parent side preserves it), but + # nothing on the subprocess child path ever reads it. Without this block, a + # subprocess-spawned agent runs as a generic session with NO persona at all + # -- the persona is silently dropped. Mirrors the in-process path in + # amplifier-app-cli's session_spawner.py (search "Inject agent's system + # instruction" near spawn_sub_session()) -- keep the two in sync. + system_instruction = config.get("instruction") + if not system_instruction: + _system_cfg = config.get("system") + if isinstance(_system_cfg, dict): + system_instruction = _system_cfg.get("instruction") + + if system_instruction: + if _mention_resolver is not None: + # Fresh deduplicator for this call -- do NOT share the session's + # instance with the prompt expansion below. ContentDeduplicator + # accumulates every file it has ever seen and re-serializes the + # whole set on each call, so a shared instance makes the persona's + # resolved @mention content bleed into the prompt's context block + # (verified: shared -> bleed, fresh -> no bleed). Matches the + # already-correct pattern in bundle/_prepared.py ("Fresh + # deduplicator each call"). + system_instruction = await expand_mentions_in_instruction( + system_instruction, + resolver=_mention_resolver, + deduplicator=ContentDeduplicator(), + relative_to=_relative_to, + ) + + context = session.coordinator.get("context") + if context is not None and hasattr(context, "set_system_prompt_factory"): + # Register a factory rather than a static system message so hooks + # that compose onto the system prompt have a surface to wrap. + # Mirrors the in-process spawn path exactly (see comment above). + _resolved_system_instruction = system_instruction + + async def _system_prompt_factory() -> str: + return _resolved_system_instruction + + await context.set_system_prompt_factory(_system_prompt_factory) + elif context is not None and hasattr(context, "add_message"): + await context.add_message({"role": "system", "content": system_instruction}) + + # (7) Expand @mentions in the prompt before execute. Mirrors PreparedBundle.spawn. if _mention_resolver is not None: prompt = await expand_mentions_in_instruction( prompt, diff --git a/tests/test_subprocess_runner.py b/tests/test_subprocess_runner.py index e592ccfb..38d12d33 100644 --- a/tests/test_subprocess_runner.py +++ b/tests/test_subprocess_runner.py @@ -474,6 +474,340 @@ async def test_module_resolver_mounted_when_module_paths_provided( ) +class TestChildSystemInstruction: + """Tests that _run_child_session registers the agent's system instruction (persona). + + Without this, subprocess-spawned agents silently drop their persona entirely -- + the `instruction` key crosses the process boundary intact inside `config`, but + nothing on the subprocess child path ever reads it. Mirrors the in-process path + in amplifier-app-cli's session_spawner.py (spawn_sub_session()). + """ + + def _make_config_file( + self, tmp_path: Any, config: dict[str, Any], prompt: str = "Hello" + ) -> str: + config_file = tmp_path / "config.json" + config_file.write_text( + serialize_subprocess_config( + config=config, + prompt=prompt, + parent_id="parent-123", + project_path=str(tmp_path), + ) + ) + return str(config_file) + + @pytest.mark.asyncio + async def test_system_instruction_registers_factory(self, tmp_path: Any) -> None: + """LOAD-BEARING: top-level `instruction` -> set_system_prompt_factory is awaited, + and awaiting the registered factory returns exactly the instruction text. + + This is the test that must fail without the fix (proves subprocess children + otherwise run with NO persona at all). + """ + config: dict[str, Any] = { + "provider": "anthropic", + "instruction": "You are ROB-PERSONA-MARKER-9x7. Always be helpful.", + } + config_file = self._make_config_file(tmp_path, config) + + with patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession: + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + + mock_context = MagicMock() + mock_context.set_system_prompt_factory = AsyncMock() + mock_instance.coordinator.get = MagicMock( + side_effect=lambda name: mock_context if name == "context" else None + ) + MockSession.return_value = mock_instance + + await _run_child_session(config_file) + + mock_context.set_system_prompt_factory.assert_awaited_once() + registered_factory = mock_context.set_system_prompt_factory.await_args[0][0] + assert ( + await registered_factory() + == "You are ROB-PERSONA-MARKER-9x7. Always be helpful." + ) + # add_message must NOT have been used -- factory path takes precedence. + assert ( + not hasattr(mock_context, "add_message") + or not mock_context.add_message.called + ) + + @pytest.mark.asyncio + async def test_system_instruction_fallback_add_message_when_no_factory( + self, tmp_path: Any + ) -> None: + """Context WITHOUT set_system_prompt_factory -> falls back to add_message.""" + config: dict[str, Any] = { + "provider": "anthropic", + "instruction": "Fallback persona text", + } + config_file = self._make_config_file(tmp_path, config) + + with patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession: + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + + # spec=["add_message"] means hasattr(context, "set_system_prompt_factory") + # is False -- no factory surface available on this context. + mock_context = MagicMock(spec=["add_message"]) + mock_context.add_message = AsyncMock() + mock_instance.coordinator.get = MagicMock( + side_effect=lambda name: mock_context if name == "context" else None + ) + MockSession.return_value = mock_instance + + await _run_child_session(config_file) + + mock_context.add_message.assert_awaited_once_with( + {"role": "system", "content": "Fallback persona text"} + ) + + @pytest.mark.asyncio + async def test_system_instruction_nested_system_dict_form( + self, tmp_path: Any + ) -> None: + """Nested config["system"]["instruction"] form (no top-level instruction) -> registered.""" + config: dict[str, Any] = { + "provider": "anthropic", + "system": {"instruction": "Nested persona text"}, + } + config_file = self._make_config_file(tmp_path, config) + + with patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession: + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + + mock_context = MagicMock() + mock_context.set_system_prompt_factory = AsyncMock() + mock_instance.coordinator.get = MagicMock( + side_effect=lambda name: mock_context if name == "context" else None + ) + MockSession.return_value = mock_instance + + await _run_child_session(config_file) + + mock_context.set_system_prompt_factory.assert_awaited_once() + registered_factory = mock_context.set_system_prompt_factory.await_args[0][0] + assert await registered_factory() == "Nested persona text" + + @pytest.mark.asyncio + async def test_no_system_instruction_registers_nothing(self, tmp_path: Any) -> None: + """No instruction anywhere -> neither set_system_prompt_factory nor add_message + is called. Proves we don't inject an empty/placeholder persona. + """ + config: dict[str, Any] = {"provider": "anthropic"} + config_file = self._make_config_file(tmp_path, config) + + with patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession: + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + + mock_context = MagicMock() + mock_context.set_system_prompt_factory = AsyncMock() + mock_context.add_message = AsyncMock() + mock_instance.coordinator.get = MagicMock( + side_effect=lambda name: mock_context if name == "context" else None + ) + MockSession.return_value = mock_instance + + await _run_child_session(config_file) + + mock_context.set_system_prompt_factory.assert_not_awaited() + mock_context.add_message.assert_not_awaited() + + @pytest.mark.asyncio + async def test_system_instruction_mentions_expanded(self, tmp_path: Any) -> None: + """Instruction containing an @mention -> the REGISTERED text is the expanded + form, not the raw string. + """ + config: dict[str, Any] = { + "provider": "anthropic", + "instruction": "Follow @some/doc.md carefully.", + } + config_file = self._make_config_file(tmp_path, config) + + expanded_text = ( + "expanded content\n\n" + "Follow @some/doc.md carefully." + ) + + with ( + patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession, + patch( + "amplifier_foundation.mentions.expand_mentions_in_instruction", + new=AsyncMock(return_value=expanded_text), + ) as mock_expand, + ): + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + # A real (truthy) mention_resolver so the expansion branch executes. + mock_instance.coordinator.get_capability = MagicMock( + side_effect=lambda name: ( + "fake-resolver" if name == "mention_resolver" else None + ) + ) + + mock_context = MagicMock() + mock_context.set_system_prompt_factory = AsyncMock() + mock_instance.coordinator.get = MagicMock( + side_effect=lambda name: mock_context if name == "context" else None + ) + MockSession.return_value = mock_instance + + await _run_child_session(config_file) + + assert mock_expand.await_count >= 1 + mock_context.set_system_prompt_factory.assert_awaited_once() + registered_factory = mock_context.set_system_prompt_factory.await_args[0][0] + assert await registered_factory() == expanded_text + + @pytest.mark.asyncio + async def test_context_none_no_crash(self, tmp_path: Any) -> None: + """Coordinator.get("context") returning None must not crash the child session.""" + config: dict[str, Any] = { + "provider": "anthropic", + "instruction": "Some persona text", + } + config_file = self._make_config_file(tmp_path, config) + + with patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession: + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + mock_instance.coordinator.get = MagicMock(return_value=None) + MockSession.return_value = mock_instance + + result = await _run_child_session(config_file) + + assert result == "result" + + @pytest.mark.asyncio + async def test_persona_mentions_do_not_bleed_into_prompt( + self, tmp_path: Any + ) -> None: + """REGRESSION: persona @mention expansion must use a FRESH ContentDeduplicator, + not the shared session capability -- otherwise ContentDeduplicator.get_unique_files() + (which returns EVERY file the instance has ever seen, not just what was resolved on + this call) makes the persona's resolved @mention content bleed into the prompt's + expanded context block. + + Uses the REAL expand_mentions_in_instruction + REAL ContentDeduplicator (nothing + mocked at that layer) with a real capability store on the mocked coordinator, so + register_capability("mention_deduplicator", ...) / get_capability(...) round-trip + exactly like the real coordinator does. + """ + persona_file = tmp_path / "persona.md" + persona_file.write_text("PERSONA-FILE-CONTENT-XYZ") + task_file = tmp_path / "task.md" + task_file.write_text("TASK-FILE-CONTENT-ABC") + + mention_map = {"@persona.md": persona_file, "@task.md": task_file} + + class _FakeResolver: + def resolve(self, mention: str) -> Path | None: + return mention_map.get(mention) + + config: dict[str, Any] = { + "provider": "anthropic", + "instruction": "Follow @persona.md carefully.", + } + config_file = self._make_config_file( + tmp_path, config, prompt="Complete @task.md now." + ) + + with patch( + "amplifier_foundation.subprocess_runner.AmplifierSession" + ) as MockSession: + mock_instance = MagicMock() + mock_instance.initialize = AsyncMock() + mock_instance.execute = AsyncMock(return_value="result") + mock_instance.cleanup = AsyncMock() + mock_instance.coordinator = MagicMock() + mock_instance.coordinator.mount = AsyncMock() + + # Real capability store so register_capability(...) writes what + # get_capability(...) reads back -- mirrors the real coordinator's + # round-trip instead of hand-waving a canned return value. + capability_store: dict[str, Any] = {} + + def _register_capability(name: str, value: Any) -> None: + capability_store[name] = value + + def _get_capability(name: str) -> Any: + if name == "mention_resolver": + return _FakeResolver() + return capability_store.get(name) + + mock_instance.coordinator.register_capability = MagicMock( + side_effect=_register_capability + ) + mock_instance.coordinator.get_capability = MagicMock( + side_effect=_get_capability + ) + + mock_context = MagicMock() + mock_context.set_system_prompt_factory = AsyncMock() + mock_instance.coordinator.get = MagicMock( + side_effect=lambda name: mock_context if name == "context" else None + ) + MockSession.return_value = mock_instance + + await _run_child_session(config_file) + + # Sanity: persona expansion itself worked (its own mention resolved). + mock_context.set_system_prompt_factory.assert_awaited_once() + registered_factory = mock_context.set_system_prompt_factory.await_args[0][0] + persona_expanded = await registered_factory() + assert "PERSONA-FILE-CONTENT-XYZ" in persona_expanded + + # The prompt passed to session.execute() must have its OWN mention + # resolved (proves the test setup is live, not vacuously passing)... + mock_instance.execute.assert_awaited_once() + prompt_arg = mock_instance.execute.await_args[0][0] + assert "TASK-FILE-CONTENT-ABC" in prompt_arg + # ...but must NOT contain the persona's resolved content (no bleed-through). + assert "PERSONA-FILE-CONTENT-XYZ" not in prompt_arg + + class TestMainEntryPoint: """Tests for the __main__ entry point."""