mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-27 21:05:34 +00:00
feat(ask): wire SystemPromptBuilder so SOUL.md / MEMORY.md / USER.md actually load
`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 <name>` 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) <noreply@anthropic.com>
This commit is contained in:
committed by
krypticmouse
co-authored by
Claude Opus 4.7
parent
0217a901dd
commit
90f009e906
@@ -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()
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user