From 90f009e9062886197b4db1775e34c19cb3a85490 Mon Sep 17 00:00:00 2001 From: IsaacH Date: Thu, 30 Apr 2026 11:53:22 +0800 Subject: [PATCH] feat(ask): wire SystemPromptBuilder so SOUL.md / MEMORY.md / USER.md actually load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SystemPromptBuilder` is fully implemented and tested in `openjarvis.prompt.builder`, and `BaseAgent.__init__` accepts a `prompt_builder` kwarg. But no production code path ever instantiates the builder, so the persona-files feature documented in `MemoryFilesConfig` (SOUL.md / MEMORY.md / USER.md) had no effect. Users could write a fully-customized `~/.openjarvis/SOUL.md` and the file was never read. This PR wires it up in `_run_agent` (called by `jarvis ask --agent ` and the new fallback from #294): ```python if "prompt_builder" in inspect.signature(agent_cls.__init__).parameters: agent_kwargs["prompt_builder"] = SystemPromptBuilder( agent_template=config.agent.default_system_prompt or "", memory_files_config=config.memory_files, system_prompt_config=config.system_prompt, ) ``` The `inspect` guard means agents that override `__init__` without forwarding `prompt_builder` (e.g. OrchestratorAgent, which has its own tool-aware system prompt) opt out automatically and keep working unchanged. SimpleAgent and any future agent that inherits `BaseAgent.__init__` directly picks up the persona files. Also fixes a latent bug in `SystemPromptBuilder._load_file`: it called `path.read_text()` with no encoding, which on Windows falls back to the system code page (cp950 / cp932 / cp949) and raises `UnicodeDecodeError` on any non-ASCII persona content. Pin to UTF-8. ## Tests - Add `test_soul_md_content_reaches_engine_in_simple_agent` — writes sentinel SOUL.md / MEMORY.md / USER.md to tmp_path, runs the command, and asserts each sentinel appears in the SYSTEM message passed to engine.generate. - Add `test_orchestrator_keeps_its_own_system_prompt` — exercises the `inspect`-based opt-out so OrchestratorAgent doesn't crash on the unexpected kwarg. Run: `pytest tests/cli/test_ask_agent.py tests/cli/test_ask_router.py tests/cli/test_ask_e2e.py tests/agents/ tests/prompt/` The 6 remaining failures (test_base_agent, test_loop_guard, test_manager, test_native_openhands) are pre-existing on origin/main and unrelated to this change. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/openjarvis/cli/ask.py | 16 ++++++ src/openjarvis/prompt/builder.py | 5 +- tests/cli/test_ask_agent.py | 97 ++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/openjarvis/cli/ask.py b/src/openjarvis/cli/ask.py index 165d5f68..06782b8a 100644 --- a/src/openjarvis/cli/ask.py +++ b/src/openjarvis/cli/ask.py @@ -357,6 +357,22 @@ def _run_agent( if capability_policy is not None: agent_kwargs["capability_policy"] = capability_policy + # Wire the SystemPromptBuilder so SOUL.md / MEMORY.md / USER.md persona + # files actually reach the model. Only passed to agents whose __init__ + # accepts a `prompt_builder` kwarg (BaseAgent does; agents that override + # __init__ without forwarding it, e.g. OrchestratorAgent, opt out + # automatically and keep their existing system-prompt machinery). + import inspect as _inspect + + if "prompt_builder" in _inspect.signature(agent_cls.__init__).parameters: + from openjarvis.prompt.builder import SystemPromptBuilder + + agent_kwargs["prompt_builder"] = SystemPromptBuilder( + agent_template=config.agent.default_system_prompt or "", + memory_files_config=config.memory_files, + system_prompt_config=config.system_prompt, + ) + agent = agent_cls(engine, model_name, **agent_kwargs) ctx = AgentContext() diff --git a/src/openjarvis/prompt/builder.py b/src/openjarvis/prompt/builder.py index 1b562fb0..f014c3a7 100644 --- a/src/openjarvis/prompt/builder.py +++ b/src/openjarvis/prompt/builder.py @@ -86,7 +86,10 @@ class SystemPromptBuilder: path = Path(path_str).expanduser() if not path.exists(): return "" - content = path.read_text() + # Always read as UTF-8. On Windows, ``read_text()`` falls back to the + # system code page (e.g. cp950 for zh-TW, cp932 for ja) and raises + # ``UnicodeDecodeError`` on any non-ASCII persona content. + content = path.read_text(encoding="utf-8") if len(content) <= max_chars: return content return self._truncate(content, max_chars) diff --git a/tests/cli/test_ask_agent.py b/tests/cli/test_ask_agent.py index 8da0e234..a1c4358f 100644 --- a/tests/cli/test_ask_agent.py +++ b/tests/cli/test_ask_agent.py @@ -336,3 +336,100 @@ class TestBuildTools: config = JarvisConfig() tools = _build_tools(["calculator", "think"], config, mock_setup, "test-model") assert len(tools) == 2 + + +class TestPersonaFilesReachModel: + """End-to-end coverage for SOUL.md / MEMORY.md / USER.md integration. + + Without these tests, the ``SystemPromptBuilder`` and the persona files + documented in the README are present in the codebase but never reach + the model — the bug this suite is meant to prevent regressing into. + """ + + def test_soul_md_content_reaches_engine_in_simple_agent( + self, runner, monkeypatch, tmp_path + ): + """SOUL.md content must appear in the system message sent to the engine.""" + from openjarvis.core.config import JarvisConfig + + # Write a SOUL.md with a unique sentinel string we can grep for + soul = tmp_path / "SOUL.md" + soul.write_text("PERSONA_SENTINEL_zh_jarvis", encoding="utf-8") + memory = tmp_path / "MEMORY.md" + memory.write_text("MEMORY_SENTINEL", encoding="utf-8") + user = tmp_path / "USER.md" + user.write_text("USER_SENTINEL", encoding="utf-8") + + cfg = JarvisConfig() + cfg.memory_files.soul_path = str(soul) + cfg.memory_files.memory_path = str(memory) + cfg.memory_files.user_path = str(user) + cfg.agent.default_system_prompt = "BASELINE_TEMPLATE" + # Disable memory context injection so it doesn't add another SYSTEM + # message and confuse the assertion. + cfg.agent.context_from_memory = False + + engine = _mock_engine() + _register_agents() + _register_tools() + with ( + patch.object(_ask_mod, "load_config", return_value=cfg), + patch.object( + _ask_mod, "get_engine", return_value=("mock", engine) + ), + patch.object(_ask_mod, "discover_engines", return_value=[("mock", engine)]), + patch.object( + _ask_mod, "discover_models", return_value={"mock": ["test-model"]} + ), + patch.object(_ask_mod, "register_builtin_models"), + patch.object(_ask_mod, "merge_discovered_models"), + ): + result = runner.invoke(cli, ["ask", "--agent", "simple", "Hello"]) + + assert result.exit_code == 0, result.output + # Grab the messages passed to engine.generate + engine.generate.assert_called() + call_args = engine.generate.call_args + messages = call_args.args[0] if call_args.args else call_args.kwargs.get("messages") + assert messages is not None and len(messages) >= 2 + system_messages = [m for m in messages if str(m.role).endswith("SYSTEM")] + assert system_messages, f"No SYSTEM message in {messages!r}" + joined = "\n".join(m.content for m in system_messages) + assert "BASELINE_TEMPLATE" in joined + assert "PERSONA_SENTINEL_zh_jarvis" in joined + assert "MEMORY_SENTINEL" in joined + assert "USER_SENTINEL" in joined + + def test_orchestrator_keeps_its_own_system_prompt( + self, runner, monkeypatch, tmp_path + ): + """OrchestratorAgent's __init__ doesn't accept ``prompt_builder``; + the wiring must skip it silently rather than crash.""" + from openjarvis.core.config import JarvisConfig + + soul = tmp_path / "SOUL.md" + soul.write_text("ORCH_PERSONA_SENTINEL", encoding="utf-8") + + cfg = JarvisConfig() + cfg.memory_files.soul_path = str(soul) + cfg.agent.context_from_memory = False + + engine = _mock_engine() + _register_agents() + _register_tools() + with ( + patch.object(_ask_mod, "load_config", return_value=cfg), + patch.object( + _ask_mod, "get_engine", return_value=("mock", engine) + ), + patch.object(_ask_mod, "discover_engines", return_value=[("mock", engine)]), + patch.object( + _ask_mod, "discover_models", return_value={"mock": ["test-model"]} + ), + patch.object(_ask_mod, "register_builtin_models"), + patch.object(_ask_mod, "merge_discovered_models"), + ): + result = runner.invoke(cli, ["ask", "--agent", "orchestrator", "Hello"]) + + # Pass condition: doesn't crash with TypeError on prompt_builder kwarg. + assert result.exit_code == 0, result.output