mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-27 21:05:34 +00:00
fix(server): load SOUL.md / USER.md context in streaming chat (#449)
* fix(server): load SOUL.md / USER.md context in streaming chat * refactor+test: extract _build_managed_system_prompt + cover #431 The streaming persona fix was inline and untestable without a live engine. Extract it into _build_managed_system_prompt (matching this module's extract-and-unit-test pattern for the streaming helpers) and add regression tests: - SOUL.md persona is injected into the streaming system prompt (#431), - the agent's own template is preserved, - output matches a directly-constructed SystemPromptBuilder (parity with the CLI/ask path — the whole point of the fix). Behavior unchanged from the original PR; this only makes it testable and locks in CLI parity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Jon Saad-Falcon <41205309+jonsaadfalcon@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Arush Wadhawan
co-authored by
Jon Saad-Falcon
Claude Opus 4.8
Jon Saad-Falcon
Claude Opus 4.8
parent
b5926400e2
commit
09b19193fe
@@ -439,6 +439,26 @@ _SAMPLER_PARAM_KEYS = (
|
||||
)
|
||||
|
||||
|
||||
def _build_managed_system_prompt(system_prompt: str, app_config: Any) -> str:
|
||||
"""Build the streaming managed-agent system prompt via SystemPromptBuilder.
|
||||
|
||||
Routes the agent's own ``system_prompt`` through the same builder the
|
||||
CLI/ask path uses, so SOUL.md / MEMORY.md / USER.md persona files are
|
||||
injected for streaming chat too (#431). Returns the assembled prompt
|
||||
(caller decides whether to append a SYSTEM message); an agent with
|
||||
neither persona nor template yields an empty string, preserving the
|
||||
prior no-SYSTEM-message behavior.
|
||||
"""
|
||||
from openjarvis.prompt.builder import SystemPromptBuilder
|
||||
|
||||
builder = SystemPromptBuilder(
|
||||
agent_template=system_prompt or "",
|
||||
memory_files_config=getattr(app_config, "memory_files", None),
|
||||
system_prompt_config=getattr(app_config, "system_prompt", None),
|
||||
)
|
||||
return builder.build()
|
||||
|
||||
|
||||
def _sampler_kwargs(config: Dict[str, Any]) -> Dict[str, Any]:
|
||||
"""Extract per-agent sampler params from a managed agent's config (#386)."""
|
||||
out: Dict[str, Any] = {}
|
||||
@@ -811,8 +831,23 @@ async def _stream_managed_agent(
|
||||
|
||||
# Build conversation messages from history + current input
|
||||
llm_messages: List[Message] = []
|
||||
if system_prompt:
|
||||
llm_messages.append(Message(role=Role.SYSTEM, content=system_prompt))
|
||||
|
||||
# Wire the SystemPromptBuilder to inject SOUL.md / MEMORY.md / USER.md
|
||||
# persona files (parity with the CLI/ask path) — see #431.
|
||||
app_config = getattr(app_state, "config", None)
|
||||
if app_config is None:
|
||||
from openjarvis.core.config import load_config
|
||||
|
||||
app_config = load_config()
|
||||
|
||||
final_system_prompt = _build_managed_system_prompt(
|
||||
system_prompt or "", app_config
|
||||
)
|
||||
|
||||
if final_system_prompt and final_system_prompt.strip():
|
||||
llm_messages.append(
|
||||
Message(role=Role.SYSTEM, content=final_system_prompt.strip())
|
||||
)
|
||||
|
||||
# Resolve agent type and class for DeepResearch tool wiring
|
||||
agent_type = agent_record.get("agent_type", "")
|
||||
|
||||
@@ -15,6 +15,7 @@ pytest.importorskip("fastapi") # agent_manager_routes imports FastAPI at module
|
||||
|
||||
from openjarvis.core.types import Role # noqa: E402
|
||||
from openjarvis.server.agent_manager_routes import ( # noqa: E402
|
||||
_build_managed_system_prompt,
|
||||
_instantiate_managed_tool,
|
||||
_replay_history_messages,
|
||||
_sampler_kwargs,
|
||||
@@ -26,12 +27,26 @@ class TestReplayHistoryToolCalls:
|
||||
|
||||
def test_assistant_tool_calls_are_replayed_with_results(self):
|
||||
history = [ # DESC order (newest first), as list_messages returns
|
||||
{"id": "m2", "direction": "agent_to_user", "content": "", "tool_calls": [
|
||||
{"tool": "shell_exec", "arguments": '{"command":"pwd"}',
|
||||
"result": "/home/u", "success": True, "latency": 1.0},
|
||||
]},
|
||||
{"id": "m1", "direction": "user_to_agent", "content": "run pwd",
|
||||
"tool_calls": None},
|
||||
{
|
||||
"id": "m2",
|
||||
"direction": "agent_to_user",
|
||||
"content": "",
|
||||
"tool_calls": [
|
||||
{
|
||||
"tool": "shell_exec",
|
||||
"arguments": '{"command":"pwd"}',
|
||||
"result": "/home/u",
|
||||
"success": True,
|
||||
"latency": 1.0,
|
||||
},
|
||||
],
|
||||
},
|
||||
{
|
||||
"id": "m1",
|
||||
"direction": "user_to_agent",
|
||||
"content": "run pwd",
|
||||
"tool_calls": None,
|
||||
},
|
||||
]
|
||||
msgs = _replay_history_messages(history, exclude_id="current")
|
||||
# Chronological: user, assistant(tool_calls), tool(result)
|
||||
@@ -46,8 +61,12 @@ class TestReplayHistoryToolCalls:
|
||||
|
||||
def test_plain_assistant_without_tool_calls(self):
|
||||
history = [
|
||||
{"id": "m1", "direction": "agent_to_user", "content": "hi",
|
||||
"tool_calls": None},
|
||||
{
|
||||
"id": "m1",
|
||||
"direction": "agent_to_user",
|
||||
"content": "hi",
|
||||
"tool_calls": None,
|
||||
},
|
||||
]
|
||||
msgs = _replay_history_messages(history, exclude_id="current")
|
||||
assert len(msgs) == 1
|
||||
@@ -56,10 +75,18 @@ class TestReplayHistoryToolCalls:
|
||||
|
||||
def test_excludes_current_message(self):
|
||||
history = [
|
||||
{"id": "cur", "direction": "user_to_agent", "content": "now",
|
||||
"tool_calls": None},
|
||||
{"id": "old", "direction": "user_to_agent", "content": "before",
|
||||
"tool_calls": None},
|
||||
{
|
||||
"id": "cur",
|
||||
"direction": "user_to_agent",
|
||||
"content": "now",
|
||||
"tool_calls": None,
|
||||
},
|
||||
{
|
||||
"id": "old",
|
||||
"direction": "user_to_agent",
|
||||
"content": "before",
|
||||
"tool_calls": None,
|
||||
},
|
||||
]
|
||||
msgs = _replay_history_messages(history, exclude_id="cur")
|
||||
assert [m.content for m in msgs] == ["before"]
|
||||
@@ -69,13 +96,24 @@ class TestSamplerKwargs:
|
||||
"""#386 — sampler params forwarded only when set."""
|
||||
|
||||
def test_reads_present_keys(self):
|
||||
cfg = {"temperature": 0.7, "repetition_penalty": 1.1, "top_p": 0.9,
|
||||
"top_k": 40, "min_p": 0.05, "frequency_penalty": 0.2,
|
||||
"presence_penalty": 0.1}
|
||||
cfg = {
|
||||
"temperature": 0.7,
|
||||
"repetition_penalty": 1.1,
|
||||
"top_p": 0.9,
|
||||
"top_k": 40,
|
||||
"min_p": 0.05,
|
||||
"frequency_penalty": 0.2,
|
||||
"presence_penalty": 0.1,
|
||||
}
|
||||
out = _sampler_kwargs(cfg)
|
||||
assert out == {"repetition_penalty": 1.1, "top_p": 0.9, "top_k": 40,
|
||||
"min_p": 0.05, "frequency_penalty": 0.2,
|
||||
"presence_penalty": 0.1}
|
||||
assert out == {
|
||||
"repetition_penalty": 1.1,
|
||||
"top_p": 0.9,
|
||||
"top_k": 40,
|
||||
"min_p": 0.05,
|
||||
"frequency_penalty": 0.2,
|
||||
"presence_penalty": 0.1,
|
||||
}
|
||||
# temperature/max_tokens are handled separately, not here.
|
||||
assert "temperature" not in out
|
||||
|
||||
@@ -98,34 +136,107 @@ class TestInstantiateManagedTool:
|
||||
|
||||
def test_memory_tool_gets_backend(self):
|
||||
backend = object()
|
||||
app_state = SimpleNamespace(memory_backend=backend, config=None,
|
||||
channel_bridge=None)
|
||||
tool = _instantiate_managed_tool(_FakeTool, "memory_store",
|
||||
engine=object(), model="m",
|
||||
app_state=app_state)
|
||||
app_state = SimpleNamespace(
|
||||
memory_backend=backend, config=None, channel_bridge=None
|
||||
)
|
||||
tool = _instantiate_managed_tool(
|
||||
_FakeTool, "memory_store", engine=object(), model="m", app_state=app_state
|
||||
)
|
||||
assert tool.kwargs == {"backend": backend}
|
||||
|
||||
def test_llm_tool_gets_engine_and_model(self):
|
||||
engine = object()
|
||||
app_state = SimpleNamespace(memory_backend=None, config=None,
|
||||
channel_bridge=None)
|
||||
tool = _instantiate_managed_tool(_FakeTool, "llm", engine=engine,
|
||||
model="qwen", app_state=app_state)
|
||||
app_state = SimpleNamespace(
|
||||
memory_backend=None, config=None, channel_bridge=None
|
||||
)
|
||||
tool = _instantiate_managed_tool(
|
||||
_FakeTool, "llm", engine=engine, model="qwen", app_state=app_state
|
||||
)
|
||||
assert tool.kwargs == {"engine": engine, "model": "qwen"}
|
||||
|
||||
def test_channel_tool_gets_channel(self):
|
||||
bridge = object()
|
||||
app_state = SimpleNamespace(memory_backend=None, config=None,
|
||||
channel_bridge=bridge)
|
||||
tool = _instantiate_managed_tool(_FakeTool, "channel_send",
|
||||
engine=object(), model="m",
|
||||
app_state=app_state)
|
||||
app_state = SimpleNamespace(
|
||||
memory_backend=None, config=None, channel_bridge=bridge
|
||||
)
|
||||
tool = _instantiate_managed_tool(
|
||||
_FakeTool, "channel_send", engine=object(), model="m", app_state=app_state
|
||||
)
|
||||
assert tool.kwargs == {"channel": bridge}
|
||||
|
||||
def test_plain_tool_gets_no_injection(self):
|
||||
app_state = SimpleNamespace(memory_backend=None, config=None,
|
||||
channel_bridge=None)
|
||||
tool = _instantiate_managed_tool(_FakeTool, "calculator",
|
||||
engine=object(), model="m",
|
||||
app_state=app_state)
|
||||
app_state = SimpleNamespace(
|
||||
memory_backend=None, config=None, channel_bridge=None
|
||||
)
|
||||
tool = _instantiate_managed_tool(
|
||||
_FakeTool, "calculator", engine=object(), model="m", app_state=app_state
|
||||
)
|
||||
assert tool.kwargs == {}
|
||||
|
||||
|
||||
class TestBuildManagedSystemPrompt:
|
||||
"""#431 — the streaming managed-agent path must run the agent's
|
||||
system_prompt through SystemPromptBuilder so SOUL.md / MEMORY.md /
|
||||
USER.md persona files are injected (parity with the CLI/ask path),
|
||||
instead of using the raw config system_prompt verbatim.
|
||||
"""
|
||||
|
||||
def _config_with_soul(self, tmp_path):
|
||||
from openjarvis.core.config import (
|
||||
MemoryFilesConfig,
|
||||
SystemPromptConfig,
|
||||
)
|
||||
|
||||
soul = tmp_path / "SOUL.md"
|
||||
soul.write_text("You are Jarvis, a meticulous local-first assistant.")
|
||||
# Other persona files point at non-existent paths — only SOUL is set.
|
||||
return SimpleNamespace(
|
||||
memory_files=MemoryFilesConfig(soul_path=str(soul)),
|
||||
system_prompt=SystemPromptConfig(),
|
||||
)
|
||||
|
||||
def test_injects_soul_persona(self, tmp_path):
|
||||
app_config = self._config_with_soul(tmp_path)
|
||||
result = _build_managed_system_prompt(
|
||||
system_prompt="You are a helpful assistant.",
|
||||
app_config=app_config,
|
||||
)
|
||||
# The persona file content is injected (the #431 fix)...
|
||||
assert "meticulous local-first assistant" in result
|
||||
# ...alongside the agent's own template.
|
||||
assert "You are a helpful assistant." in result
|
||||
|
||||
def test_agent_template_is_preserved(self, tmp_path):
|
||||
from openjarvis.core.config import MemoryFilesConfig, SystemPromptConfig
|
||||
|
||||
# No persona files configured (all default paths).
|
||||
app_config = SimpleNamespace(
|
||||
memory_files=MemoryFilesConfig(),
|
||||
system_prompt=SystemPromptConfig(),
|
||||
)
|
||||
result = _build_managed_system_prompt(
|
||||
system_prompt="Plain agent.",
|
||||
app_config=app_config,
|
||||
)
|
||||
# The agent's own template is carried into the assembled prompt.
|
||||
assert "Plain agent." in result
|
||||
|
||||
def test_matches_cli_builder_output(self, tmp_path):
|
||||
"""Parity check: the helper produces exactly what a directly-
|
||||
constructed SystemPromptBuilder produces (same path the CLI uses),
|
||||
so streaming chat and `jarvis ask` assemble the prompt identically.
|
||||
"""
|
||||
from openjarvis.core.config import MemoryFilesConfig, SystemPromptConfig
|
||||
from openjarvis.prompt.builder import SystemPromptBuilder
|
||||
|
||||
app_config = SimpleNamespace(
|
||||
memory_files=MemoryFilesConfig(),
|
||||
system_prompt=SystemPromptConfig(),
|
||||
)
|
||||
helper_out = _build_managed_system_prompt("Agent X.", app_config)
|
||||
direct_out = SystemPromptBuilder(
|
||||
agent_template="Agent X.",
|
||||
memory_files_config=app_config.memory_files,
|
||||
system_prompt_config=app_config.system_prompt,
|
||||
).build()
|
||||
assert helper_out == direct_out
|
||||
|
||||
Reference in New Issue
Block a user