mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-08-16 09:51:59 +00:00
Compare commits
3
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
03c5ec3e40 | ||
|
|
4218258486 | ||
|
|
176ed3029b |
@@ -60,7 +60,27 @@ class BaseChannel(ABC):
|
||||
conversation_id: str = "",
|
||||
metadata: Dict[str, Any] | None = None,
|
||||
) -> bool:
|
||||
"""Send a message to a specific channel. Returns True on success."""
|
||||
"""Send a message to a specific channel. Returns True on success.
|
||||
|
||||
Canonical send contract shared by **every** channel adapter:
|
||||
|
||||
``channel``
|
||||
The DESTINATION identifier — the per-adapter native id of the
|
||||
place the message goes (Discord/Slack channel id, Telegram chat
|
||||
id, email recipient address, ...). This is *not* the channel
|
||||
TYPE label. An incoming :class:`ChannelMessage` carries that
|
||||
destination in its ``conversation_id`` field (``channel`` there
|
||||
is only the type label such as ``"discord"``), so dispatch code
|
||||
replying to a message must pass ``cm.conversation_id`` here.
|
||||
``conversation_id``
|
||||
An optional reply/thread reference — the native id of the
|
||||
message being replied to (Discord ``message_reference``, Slack
|
||||
``thread_ts``, Telegram ``reply_to_message_id``, email
|
||||
``In-Reply-To``, ...). When replying to an inbound message this
|
||||
should be ``cm.message_id``, never the channel id. Passing a
|
||||
channel id here yields broken references (e.g. Discord
|
||||
``MESSAGE_REFERENCE_UNKNOWN_MESSAGE``).
|
||||
"""
|
||||
|
||||
@abstractmethod
|
||||
def status(self) -> ChannelStatus:
|
||||
|
||||
@@ -112,7 +112,15 @@ class TelegramChannel(BaseChannel):
|
||||
|
||||
_TELEGRAM_MAX_LEN = 4096
|
||||
url = f"https://api.telegram.org/bot{self._token}/sendMessage"
|
||||
chat_id = conversation_id or channel
|
||||
# Canonical channel send contract (see BaseChannel.send): the first
|
||||
# positional ``channel`` arg is the DESTINATION (the Telegram chat
|
||||
# id). ``conversation_id`` is the inbound message id used as a
|
||||
# reply/thread reference (``reply_to_message_id``). We fall back to
|
||||
# ``conversation_id`` as the chat id only when ``channel`` is empty,
|
||||
# for backwards compatibility with legacy callers that passed the
|
||||
# chat id via ``conversation_id``.
|
||||
chat_id = channel or conversation_id
|
||||
reply_to = conversation_id if (channel and conversation_id) else ""
|
||||
chunks = textwrap.wrap(
|
||||
content,
|
||||
width=_TELEGRAM_MAX_LEN,
|
||||
@@ -126,6 +134,8 @@ class TelegramChannel(BaseChannel):
|
||||
}
|
||||
if self._parse_mode:
|
||||
payload["parse_mode"] = self._parse_mode
|
||||
if reply_to:
|
||||
payload["reply_to_message_id"] = reply_to
|
||||
|
||||
resp = httpx.post(url, json=payload, timeout=10.0)
|
||||
if resp.status_code >= 300:
|
||||
|
||||
@@ -28,12 +28,22 @@ def _read_input(prompt: str = "You> ") -> Optional[str]:
|
||||
@click.option("-a", "--agent", "agent_name", default=None, help="Agent type.")
|
||||
@click.option("--tools", default=None, help="Comma-separated tool names.")
|
||||
@click.option("--system", "system_prompt", default=None, help="Custom system prompt.")
|
||||
@click.option(
|
||||
"--persona",
|
||||
"persona_name",
|
||||
default=None,
|
||||
help=(
|
||||
"Named persona dir under ~/.openjarvis/personas/<name>/ "
|
||||
"(overrides config). Pass 'none' to disable all persona files."
|
||||
),
|
||||
)
|
||||
def chat(
|
||||
engine_key: str | None,
|
||||
model_name: str | None,
|
||||
agent_name: str | None,
|
||||
tools: str | None,
|
||||
system_prompt: str | None,
|
||||
persona_name: str | None,
|
||||
) -> None:
|
||||
"""Start an interactive multi-turn chat session.
|
||||
|
||||
@@ -48,6 +58,14 @@ def chat(
|
||||
|
||||
config = load_config()
|
||||
|
||||
import dataclasses as _dc
|
||||
|
||||
effective_mf = (
|
||||
_dc.replace(config.memory_files, persona_name=persona_name)
|
||||
if persona_name is not None
|
||||
else config.memory_files
|
||||
)
|
||||
|
||||
# Resolve engine
|
||||
from openjarvis.engine import get_engine
|
||||
from openjarvis.intelligence import register_builtin_models
|
||||
@@ -121,6 +139,21 @@ def chat(
|
||||
|
||||
kwargs["interactive"] = True
|
||||
kwargs["confirm_callback"] = _confirm
|
||||
|
||||
import inspect as _inspect
|
||||
|
||||
if (
|
||||
"prompt_builder"
|
||||
in _inspect.signature(agent_cls.__init__).parameters
|
||||
):
|
||||
from openjarvis.prompt.builder import SystemPromptBuilder
|
||||
|
||||
kwargs["prompt_builder"] = SystemPromptBuilder(
|
||||
agent_template=config.agent.default_system_prompt or "",
|
||||
memory_files_config=effective_mf,
|
||||
system_prompt_config=config.system_prompt,
|
||||
)
|
||||
|
||||
agent = agent_cls(engine, model, **kwargs)
|
||||
except Exception as exc:
|
||||
console.print(f"[yellow]Agent '{agent_key}' failed: {exc}[/yellow]")
|
||||
@@ -147,6 +180,16 @@ def chat(
|
||||
_notifications = NotificationDispatcher(get_status())
|
||||
|
||||
# Conversation state
|
||||
if not system_prompt:
|
||||
from openjarvis.prompt.builder import SystemPromptBuilder
|
||||
|
||||
builder = SystemPromptBuilder(
|
||||
agent_template=config.agent.default_system_prompt or "",
|
||||
memory_files_config=effective_mf,
|
||||
system_prompt_config=config.system_prompt,
|
||||
)
|
||||
system_prompt = builder.build()
|
||||
|
||||
history: List[Message] = []
|
||||
if system_prompt:
|
||||
history.append(Message(role=Role.SYSTEM, content=system_prompt))
|
||||
|
||||
+73
-19
@@ -261,6 +261,11 @@ def serve(
|
||||
# Resolve agent
|
||||
agent = None
|
||||
agent_key = agent_name or config.server.agent
|
||||
# Tool instances resolved for the primary agent are reused below to build
|
||||
# the scheduler's ToolExecutor — avoiding a second full SystemBuilder.build()
|
||||
# (which would re-discover the engine, re-resolve tools, re-open the channel,
|
||||
# etc.). See the scheduler block near the bottom of this function (#263).
|
||||
resolved_tools: list = []
|
||||
if agent_key:
|
||||
try:
|
||||
import openjarvis.agents # noqa: F401
|
||||
@@ -328,6 +333,8 @@ def serve(
|
||||
|
||||
if tools:
|
||||
agent_kwargs["tools"] = tools
|
||||
# Reuse these for the scheduler's ToolExecutor (#263).
|
||||
resolved_tools = tools
|
||||
|
||||
if getattr(agent_cls, "accepts_tools", False):
|
||||
agent_kwargs["max_turns"] = config.agent.max_turns
|
||||
@@ -461,6 +468,24 @@ def serve(
|
||||
# Create app
|
||||
from openjarvis.server.app import create_app
|
||||
|
||||
# Set up memory backend for context injection. Built before the scheduler
|
||||
# block so the executor's JarvisSystem can reference it (#263).
|
||||
memory_backend = None
|
||||
if config.agent.context_from_memory:
|
||||
try:
|
||||
import openjarvis.tools.storage # noqa: F401
|
||||
from openjarvis.core.registry import MemoryRegistry
|
||||
|
||||
mem_key = config.memory.default_backend
|
||||
if MemoryRegistry.contains(mem_key):
|
||||
memory_backend = MemoryRegistry.create(
|
||||
mem_key,
|
||||
db_path=config.memory.db_path,
|
||||
)
|
||||
console.print(" Memory: [cyan]active[/cyan]")
|
||||
except Exception as exc:
|
||||
logger.debug("Memory backend init failed: %s", exc)
|
||||
|
||||
# Set up agent manager
|
||||
agent_manager = None
|
||||
if config.agent_manager.enabled:
|
||||
@@ -500,9 +525,55 @@ def serve(
|
||||
event_bus=bus,
|
||||
trace_store=_trace_store,
|
||||
)
|
||||
from openjarvis.system import SystemBuilder
|
||||
# Reuse the components already built inline above instead of a
|
||||
# second full SystemBuilder.build() — the original double-build
|
||||
# re-discovered the engine, re-instrumented it, re-resolved tools,
|
||||
# re-opened the channel and re-created the agent manager, costing
|
||||
# ~30-40s on top of an already-paid startup (#263). The executor
|
||||
# only reads engine/model/config/memory_backend/tool_executor/
|
||||
# session_store/channel_backend from the system (see
|
||||
# AgentExecutor), all of which are wired here.
|
||||
from openjarvis.sessions.session import SessionStore
|
||||
from openjarvis.system import JarvisSystem
|
||||
from openjarvis.tools._stubs import ToolExecutor
|
||||
|
||||
system = SystemBuilder(config).build()
|
||||
_sched_session_store = None
|
||||
if config.sessions.enabled:
|
||||
try:
|
||||
from pathlib import Path as _SchedPath
|
||||
|
||||
_sched_session_store = SessionStore(
|
||||
db_path=_SchedPath(config.sessions.db_path).expanduser(),
|
||||
max_age_hours=config.sessions.max_age_hours,
|
||||
consolidation_threshold=(
|
||||
config.sessions.consolidation_threshold
|
||||
),
|
||||
)
|
||||
except Exception as exc:
|
||||
logger.debug("Scheduler session store init failed: %s", exc)
|
||||
|
||||
_sched_tool_executor = (
|
||||
ToolExecutor(resolved_tools, bus) if resolved_tools else None
|
||||
)
|
||||
|
||||
system = JarvisSystem(
|
||||
config=config,
|
||||
bus=bus,
|
||||
engine=engine,
|
||||
engine_key=engine_name,
|
||||
model=model_name,
|
||||
agent=agent,
|
||||
agent_name=agent_key or "",
|
||||
tools=resolved_tools,
|
||||
tool_executor=_sched_tool_executor,
|
||||
memory_backend=memory_backend,
|
||||
telemetry_store=telem_store,
|
||||
trace_store=_trace_store,
|
||||
session_store=_sched_session_store,
|
||||
capability_policy=sec.capability_policy,
|
||||
agent_manager=agent_manager,
|
||||
agent_executor=executor,
|
||||
)
|
||||
executor.set_system(system)
|
||||
|
||||
agent_scheduler = AgentScheduler(
|
||||
@@ -522,23 +593,6 @@ def serve(
|
||||
except Exception as exc:
|
||||
logger.debug("Agent scheduler init failed: %s", exc)
|
||||
|
||||
# Set up memory backend for context injection
|
||||
memory_backend = None
|
||||
if config.agent.context_from_memory:
|
||||
try:
|
||||
import openjarvis.tools.storage # noqa: F401
|
||||
from openjarvis.core.registry import MemoryRegistry
|
||||
|
||||
mem_key = config.memory.default_backend
|
||||
if MemoryRegistry.contains(mem_key):
|
||||
memory_backend = MemoryRegistry.create(
|
||||
mem_key,
|
||||
db_path=config.memory.db_path,
|
||||
)
|
||||
console.print(" Memory: [cyan]active[/cyan]")
|
||||
except Exception as exc:
|
||||
logger.debug("Memory backend init failed: %s", exc)
|
||||
|
||||
# --- Channel Gateway: API key, sessions, ChannelBridge ---
|
||||
import os as _os
|
||||
|
||||
|
||||
@@ -229,6 +229,11 @@ class SystemPromptBuilder:
|
||||
)
|
||||
|
||||
def _load_file(self, path_str: str, max_chars: int) -> str:
|
||||
# An empty path means "no file" (e.g. the persona "none" opt-out, which
|
||||
# resolves to empty paths). Guard before Path("") — which becomes "." —
|
||||
# so reading it does not raise IsADirectoryError.
|
||||
if not path_str:
|
||||
return ""
|
||||
path = Path(path_str).expanduser()
|
||||
if not path.exists():
|
||||
return ""
|
||||
|
||||
@@ -268,10 +268,20 @@ class JarvisSystem:
|
||||
|
||||
if reply:
|
||||
try:
|
||||
# Canonical channel send contract (see BaseChannel.send):
|
||||
# the first positional arg is the DESTINATION id, and the
|
||||
# `conversation_id=` kwarg is the inbound message id used as
|
||||
# a reply/thread reference. ``cm.conversation_id`` holds the
|
||||
# real per-adapter destination (Discord/Slack channel id,
|
||||
# Telegram chat id, ...) while ``cm.channel`` is only the
|
||||
# channel TYPE label ("discord", "telegram", ...). Passing
|
||||
# the type label as the destination produced HTTP 400s
|
||||
# (#515) and using the channel id as a reply reference
|
||||
# produced MESSAGE_REFERENCE_UNKNOWN_MESSAGE (#516).
|
||||
channel_bridge.send(
|
||||
cm.channel,
|
||||
cm.conversation_id,
|
||||
reply,
|
||||
conversation_id=cm.conversation_id,
|
||||
conversation_id=getattr(cm, "message_id", ""),
|
||||
)
|
||||
except Exception:
|
||||
logger.exception("Channel send error")
|
||||
|
||||
@@ -130,3 +130,63 @@ class TestStatus:
|
||||
ch = DiscordChannel()
|
||||
ch.connect()
|
||||
assert ch.status() == ChannelStatus.ERROR
|
||||
|
||||
|
||||
class TestWireChannelEndToEnd:
|
||||
"""Regression for #515/#516 — the full inbound→reply path through
|
||||
JarvisSystem.wire_channel must call the real Discord REST API with the
|
||||
numeric channel id (not "discord") and a message_reference equal to the
|
||||
inbound message id (not the channel id).
|
||||
"""
|
||||
|
||||
def test_reply_hits_real_channel_id_and_message_reference(self, tmp_path):
|
||||
from openjarvis.channels._stubs import ChannelMessage
|
||||
from openjarvis.core.config import JarvisConfig
|
||||
from openjarvis.core.events import EventBus
|
||||
from openjarvis.system import JarvisSystem
|
||||
|
||||
config = JarvisConfig()
|
||||
config.sessions.db_path = str(tmp_path / "sessions.db")
|
||||
from unittest.mock import MagicMock as _MM
|
||||
|
||||
system = JarvisSystem(
|
||||
config=config,
|
||||
bus=EventBus(record_history=False),
|
||||
engine=_MM(),
|
||||
engine_key="mock",
|
||||
model="test-model",
|
||||
agent_name="",
|
||||
)
|
||||
system.ask = _MM(return_value={"content": "pong"})
|
||||
|
||||
channel = DiscordChannel(bot_token="my-bot-token")
|
||||
system.wire_channel(channel)
|
||||
|
||||
# Exactly the ChannelMessage shape DiscordChannel._gateway_loop emits:
|
||||
# channel = "discord" (TYPE label), conversation_id = numeric channel
|
||||
# id, message_id = numeric message id.
|
||||
cm = ChannelMessage(
|
||||
channel="discord",
|
||||
sender="user-1",
|
||||
content="hello",
|
||||
message_id="111122223333444455",
|
||||
conversation_id="987654321098765432",
|
||||
)
|
||||
|
||||
mock_response = MagicMock()
|
||||
mock_response.status_code = 200
|
||||
with patch("httpx.post", return_value=mock_response) as mock_post:
|
||||
# Invoke the handler wire_channel registered on the channel.
|
||||
for handler in channel._handlers:
|
||||
handler(cm)
|
||||
|
||||
mock_post.assert_called_once()
|
||||
url = mock_post.call_args[0][0]
|
||||
# #515: destination is the numeric channel id, not the "discord" label.
|
||||
assert "discord.com/api/v10/channels/987654321098765432/messages" in url
|
||||
assert "channels/discord/messages" not in url
|
||||
payload = mock_post.call_args[1]["json"]
|
||||
assert payload["content"] == "pong"
|
||||
# #516: message_reference is the inbound message id, NOT the channel id.
|
||||
assert payload["message_reference"] == {"message_id": "111122223333444455"}
|
||||
assert payload["message_reference"]["message_id"] != "987654321098765432"
|
||||
|
||||
@@ -105,6 +105,41 @@ class TestSend:
|
||||
event_types = [e.event_type for e in bus.history]
|
||||
assert EventType.CHANNEL_MESSAGE_SENT in event_types
|
||||
|
||||
def test_send_uses_channel_as_chat_id_under_unified_contract(self):
|
||||
"""Canonical contract (#515/#516): the first positional ``channel``
|
||||
arg is the chat destination, and ``conversation_id`` is the inbound
|
||||
message id used as ``reply_to_message_id`` — not the chat id."""
|
||||
ch = TelegramChannel(bot_token="123:ABC")
|
||||
|
||||
mock_response = MagicMock()
|
||||
mock_response.status_code = 200
|
||||
|
||||
with patch("httpx.post", return_value=mock_response) as mock_post:
|
||||
result = ch.send("12345678", "Reply!", conversation_id="55")
|
||||
assert result is True
|
||||
payload = mock_post.call_args[1]["json"]
|
||||
# Destination is the chat id from the positional channel arg.
|
||||
assert payload["chat_id"] == "12345678"
|
||||
# conversation_id becomes the reply reference, not the chat id.
|
||||
assert payload["reply_to_message_id"] == "55"
|
||||
|
||||
def test_send_legacy_conversation_id_only_still_targets_chat(self):
|
||||
"""Backwards compatibility: a legacy caller passing the chat id via
|
||||
``conversation_id`` (with an empty ``channel``) still delivers."""
|
||||
ch = TelegramChannel(bot_token="123:ABC")
|
||||
|
||||
mock_response = MagicMock()
|
||||
mock_response.status_code = 200
|
||||
|
||||
with patch("httpx.post", return_value=mock_response) as mock_post:
|
||||
result = ch.send("", "Hello!", conversation_id="12345678")
|
||||
assert result is True
|
||||
payload = mock_post.call_args[1]["json"]
|
||||
assert payload["chat_id"] == "12345678"
|
||||
# When channel is empty, conversation_id is the chat id, so it must
|
||||
# not also be used as a self-referential reply id.
|
||||
assert "reply_to_message_id" not in payload
|
||||
|
||||
|
||||
class TestStatus:
|
||||
def test_no_token_connect_error(self):
|
||||
|
||||
@@ -79,10 +79,14 @@ class TestWireChannelWithAgent:
|
||||
|
||||
system.ask.assert_called_once()
|
||||
assert system.ask.call_args[0][0] == "ping"
|
||||
# Canonical send contract (#515/#516): the destination is the real
|
||||
# per-adapter id (carried in ChannelMessage.conversation_id), not the
|
||||
# channel TYPE label, and the conversation_id kwarg is the inbound
|
||||
# message id used as a reply reference, not the channel id.
|
||||
mock_channel.send.assert_called_once_with(
|
||||
"telegram",
|
||||
"42",
|
||||
"pong",
|
||||
conversation_id="42",
|
||||
conversation_id="1",
|
||||
)
|
||||
|
||||
def test_session_store_created_lazily(self, tmp_path):
|
||||
@@ -126,13 +130,101 @@ class TestWireChannelWithEngine:
|
||||
handler = mock_channel.on_message.call_args[0][0]
|
||||
handler(_make_channel_message(content="hi"))
|
||||
|
||||
# Canonical send contract (#515/#516): destination = real channel id
|
||||
# (from ChannelMessage.conversation_id), reply ref = inbound message id.
|
||||
mock_channel.send.assert_called_once_with(
|
||||
"telegram",
|
||||
"42",
|
||||
"raw reply",
|
||||
conversation_id="42",
|
||||
conversation_id="1",
|
||||
)
|
||||
|
||||
|
||||
class TestWireChannelCanonicalContract:
|
||||
"""Regression for #515/#516 — wire_channel must dispatch the canonical
|
||||
send contract so each adapter receives the right destination/reply ids,
|
||||
regardless of the channel TYPE label.
|
||||
"""
|
||||
|
||||
def test_discord_uses_real_channel_id_not_type_label(self, tmp_path):
|
||||
"""A Discord ChannelMessage (channel="discord" TYPE label,
|
||||
conversation_id=<numeric channel id>, message_id=<numeric msg id>)
|
||||
must reply to the numeric channel id, with the message id as the
|
||||
reply reference — never "discord" as destination (#515) and never the
|
||||
channel id as the message reference (#516).
|
||||
"""
|
||||
system = _make_system(tmp_path=tmp_path)
|
||||
system.ask = MagicMock(return_value={"content": "pong"})
|
||||
|
||||
mock_channel = MagicMock()
|
||||
system.wire_channel(mock_channel)
|
||||
handler = mock_channel.on_message.call_args[0][0]
|
||||
|
||||
cm = ChannelMessage(
|
||||
channel="discord",
|
||||
sender="user-1",
|
||||
content="hello",
|
||||
message_id="111122223333444455",
|
||||
conversation_id="987654321098765432",
|
||||
)
|
||||
handler(cm)
|
||||
|
||||
args, kwargs = mock_channel.send.call_args
|
||||
# Destination is the real Discord channel id, NOT the type label.
|
||||
assert args[0] == "987654321098765432"
|
||||
assert args[0] != "discord"
|
||||
# Reply reference is the inbound message id, NOT the channel id.
|
||||
assert kwargs["conversation_id"] == "111122223333444455"
|
||||
assert kwargs["conversation_id"] != "987654321098765432"
|
||||
|
||||
def test_telegram_uses_chat_id_and_message_id(self, tmp_path):
|
||||
"""Telegram must keep working under the unified contract: destination
|
||||
is the chat id (conversation_id), reply ref is the message id."""
|
||||
system = _make_system(tmp_path=tmp_path)
|
||||
system.ask = MagicMock(return_value={"content": "pong"})
|
||||
|
||||
mock_channel = MagicMock()
|
||||
system.wire_channel(mock_channel)
|
||||
handler = mock_channel.on_message.call_args[0][0]
|
||||
|
||||
cm = ChannelMessage(
|
||||
channel="telegram",
|
||||
sender="42",
|
||||
content="ping",
|
||||
message_id="55",
|
||||
conversation_id="12345678",
|
||||
)
|
||||
handler(cm)
|
||||
|
||||
mock_channel.send.assert_called_once_with(
|
||||
"12345678",
|
||||
"pong",
|
||||
conversation_id="55",
|
||||
)
|
||||
|
||||
def test_session_key_still_uses_conversation_id(self, tmp_path):
|
||||
"""The fix must not change session isolation keying, which still uses
|
||||
``<channel>:<conversation_id>``."""
|
||||
system = _make_system(tmp_path=tmp_path)
|
||||
system.ask = MagicMock(return_value={"content": "ok"})
|
||||
|
||||
mock_channel = MagicMock()
|
||||
system.wire_channel(mock_channel)
|
||||
handler = mock_channel.on_message.call_args[0][0]
|
||||
|
||||
cm = ChannelMessage(
|
||||
channel="discord",
|
||||
sender="u1",
|
||||
content="hi",
|
||||
message_id="msg-9",
|
||||
conversation_id="chan-7",
|
||||
)
|
||||
handler(cm)
|
||||
|
||||
# Session was created under the channel:conversation_id key.
|
||||
session = system.session_store.get_or_create("discord:chan-7")
|
||||
assert any(m.content == "hi" for m in session.messages)
|
||||
|
||||
|
||||
class TestWireChannelSessionIsolation:
|
||||
"""Separate conversation_ids get independent sessions."""
|
||||
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
"""Regression tests for #263 — ``jarvis serve`` must build the system once.
|
||||
|
||||
serve.py used to construct all heavy components inline and then call
|
||||
``SystemBuilder(config).build()`` a second time inside the scheduler block,
|
||||
re-discovering the engine, re-instrumenting it, re-resolving tools, re-opening
|
||||
the channel and re-creating the agent manager — ~30-40s of redundant work.
|
||||
|
||||
These tests pin the fix:
|
||||
|
||||
1. ``SystemBuilder.build`` is never called during ``jarvis serve`` startup
|
||||
(the duplicate build is gone).
|
||||
2. The ``AgentExecutor`` still receives a system exposing the attributes it
|
||||
actually reads: ``tool_executor``, ``session_store``, ``memory_backend``,
|
||||
plus ``engine`` / ``model`` / ``config``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from click.testing import CliRunner
|
||||
|
||||
from openjarvis.cli import cli
|
||||
|
||||
pytest.importorskip("fastapi")
|
||||
pytest.importorskip("uvicorn")
|
||||
|
||||
# ``openjarvis.cli.serve`` as an attribute resolves to the click *command*
|
||||
# (re-exported on the package); grab the real module to monkeypatch its globals.
|
||||
serve_mod = importlib.import_module("openjarvis.cli.serve")
|
||||
|
||||
|
||||
def _fake_engine() -> MagicMock:
|
||||
engine = MagicMock()
|
||||
engine.list_models.return_value = ["test-model"]
|
||||
engine.health.return_value = True
|
||||
engine.name = "mock"
|
||||
return engine
|
||||
|
||||
|
||||
def _repopulate_registries() -> None:
|
||||
"""Re-run the @register decorators wiped by the autouse conftest fixture.
|
||||
|
||||
The tool/memory modules are import-cached, so a plain ``import`` inside
|
||||
serve.py is a no-op after the registries are cleared per-test. Reload the
|
||||
individual submodules so ToolRegistry/MemoryRegistry are populated exactly
|
||||
as they would be on a fresh process — otherwise serve would resolve an
|
||||
empty tool list and no memory backend, masking the very wiring under test.
|
||||
"""
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import openjarvis.agents # noqa: F401
|
||||
import openjarvis.tools # noqa: F401
|
||||
import openjarvis.tools.storage # noqa: F401
|
||||
from openjarvis.core.registry import (
|
||||
AgentRegistry,
|
||||
MemoryRegistry,
|
||||
ToolRegistry,
|
||||
)
|
||||
|
||||
if not AgentRegistry.keys():
|
||||
for mod_name in list(sys.modules):
|
||||
if mod_name.startswith("openjarvis.agents.") and not mod_name.endswith(
|
||||
"_stubs"
|
||||
):
|
||||
try:
|
||||
importlib.reload(sys.modules[mod_name])
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
if not ToolRegistry.keys():
|
||||
for mod_name in list(sys.modules):
|
||||
if (
|
||||
mod_name.startswith("openjarvis.tools.")
|
||||
and not mod_name.endswith("_stubs")
|
||||
and not mod_name.endswith("agent_tools")
|
||||
):
|
||||
try:
|
||||
importlib.reload(sys.modules[mod_name])
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
if not MemoryRegistry.keys():
|
||||
for mod_name in list(sys.modules):
|
||||
if mod_name.startswith(
|
||||
"openjarvis.tools.storage."
|
||||
) and not mod_name.endswith("_stubs"):
|
||||
try:
|
||||
importlib.reload(sys.modules[mod_name])
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
def _run_serve(tmp_path, monkeypatch, *, build_spy, set_system_spy):
|
||||
"""Invoke ``jarvis serve`` with all heavy/blocking pieces stubbed out.
|
||||
|
||||
Returns the CliRunner result. The server is never actually started
|
||||
(``uvicorn.run`` is a no-op) and no real engine is contacted.
|
||||
"""
|
||||
from openjarvis.core.config import JarvisConfig
|
||||
|
||||
_repopulate_registries()
|
||||
|
||||
config = JarvisConfig()
|
||||
# Keep the scheduler block alive (it owns the executor wiring under test)
|
||||
# while pointing every store at the temp dir.
|
||||
config.agent_manager.enabled = True
|
||||
config.agent_manager.db_path = str(tmp_path / "agents.db")
|
||||
config.sessions.enabled = True
|
||||
config.sessions.db_path = str(tmp_path / "sessions.db")
|
||||
config.memory.db_path = str(tmp_path / "memory.db")
|
||||
config.telemetry.enabled = False
|
||||
config.traces.enabled = False
|
||||
config.channel.enabled = False
|
||||
config.skills.enabled = False
|
||||
config.server.host = "127.0.0.1"
|
||||
config.server.port = 8123
|
||||
# Resolve a model without contacting a real engine / discovery.
|
||||
config.intelligence.default_model = "test-model"
|
||||
|
||||
engine = _fake_engine()
|
||||
|
||||
monkeypatch.setattr(serve_mod, "load_config", lambda *a, **k: config)
|
||||
monkeypatch.setattr(serve_mod, "get_engine", lambda *a, **k: ("mock", engine))
|
||||
monkeypatch.setattr(serve_mod, "discover_engines", lambda *a, **k: {})
|
||||
monkeypatch.setattr(serve_mod, "discover_models", lambda *a, **k: {})
|
||||
|
||||
# setup_security returns its own context; pass the engine straight through
|
||||
# so we don't need real guardrails wired up.
|
||||
sec = MagicMock()
|
||||
sec.engine = engine
|
||||
sec.capability_policy = None
|
||||
sec.audit_logger = None
|
||||
monkeypatch.setattr("openjarvis.security.setup_security", lambda *a, **k: sec)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"openjarvis.system.builder.SystemBuilder.build",
|
||||
build_spy,
|
||||
),
|
||||
patch(
|
||||
"openjarvis.agents.executor.AgentExecutor.set_system",
|
||||
set_system_spy,
|
||||
),
|
||||
patch("uvicorn.run", lambda *a, **k: None),
|
||||
):
|
||||
return CliRunner().invoke(cli, ["serve"], catch_exceptions=False)
|
||||
|
||||
|
||||
def test_serve_does_not_call_systembuilder_build(tmp_path, monkeypatch):
|
||||
"""The redundant second full build is gone (#263)."""
|
||||
build_spy = MagicMock(
|
||||
side_effect=AssertionError(
|
||||
"SystemBuilder.build() must not run during `jarvis serve` startup "
|
||||
"— it is the duplicate build #263 removed."
|
||||
)
|
||||
)
|
||||
set_system_spy = MagicMock()
|
||||
|
||||
result = _run_serve(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
build_spy=build_spy,
|
||||
set_system_spy=set_system_spy,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
build_spy.assert_not_called()
|
||||
|
||||
|
||||
def test_executor_receives_required_system_attrs(tmp_path, monkeypatch):
|
||||
"""The executor still gets a system exposing the attributes it reads.
|
||||
|
||||
AgentExecutor reads engine/model/config/memory_backend/tool_executor/
|
||||
session_store off ``self._system``; the de-dup must not strip any of them.
|
||||
"""
|
||||
build_spy = MagicMock()
|
||||
captured: dict = {}
|
||||
|
||||
def _capture_set_system(self, system): # noqa: ANN001
|
||||
captured["system"] = system
|
||||
# Preserve real behaviour so the executor is usable afterwards.
|
||||
self._system = system
|
||||
|
||||
result = _run_serve(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
build_spy=build_spy,
|
||||
set_system_spy=_capture_set_system,
|
||||
)
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
# Built once, from the inline components — not via SystemBuilder.build().
|
||||
build_spy.assert_not_called()
|
||||
|
||||
system = captured.get("system")
|
||||
assert system is not None, "executor.set_system was never called"
|
||||
|
||||
# Correctness constraint from the verifier: these must survive the de-dup.
|
||||
assert system.tool_executor is not None
|
||||
assert system.session_store is not None
|
||||
assert system.memory_backend is not None
|
||||
|
||||
# And the basics the executor resolves engine/model from.
|
||||
assert system.engine is not None
|
||||
assert system.model == "test-model"
|
||||
assert system.config is not None
|
||||
@@ -31,3 +31,23 @@ def test_named_persona_resolves_to_personas_dir():
|
||||
def test_path_traversal_rejected(bad):
|
||||
with pytest.raises(ValueError):
|
||||
SystemPromptBuilder._resolve_persona(MemoryFilesConfig(persona_name=bad))
|
||||
|
||||
|
||||
def test_none_persona_build_does_not_raise():
|
||||
"""Regression (#497): `--persona none` resolves to empty file paths; building
|
||||
the prompt must not raise IsADirectoryError when those empty paths are read
|
||||
(Path("") is "." — reading a directory raised before the empty-path guard).
|
||||
"""
|
||||
import dataclasses
|
||||
|
||||
from openjarvis.core.config import load_config
|
||||
|
||||
cfg = load_config()
|
||||
mf = dataclasses.replace(cfg.memory_files, persona_name="none")
|
||||
builder = SystemPromptBuilder(
|
||||
agent_template=cfg.agent.default_system_prompt or "",
|
||||
memory_files_config=mf,
|
||||
system_prompt_config=cfg.system_prompt,
|
||||
)
|
||||
out = builder.build()
|
||||
assert isinstance(out, str)
|
||||
|
||||
Reference in New Issue
Block a user