Consolidate top-priority contributor PRs (#235 + #339 + #293 + #294 + #295) (#362)

Lands 5 high-priority contributor PRs as one bundle, with original authorship preserved on every commit, plus follow-up commits from me to make each PR CI-green and non-regressive. See PR #362 for the full per-author commit breakdown and credits.

Credits: @tomaioo (#235), @Dilligaf371 (#339), @TX-Huang (#293, #294, #295).
This commit is contained in:
Jon Saad-Falcon
2026-05-20 13:11:34 -07:00
committed by GitHub
13 changed files with 433 additions and 12 deletions
+69 -5
View File
@@ -48,17 +48,81 @@ impl PySkillManifest {
}
fn verify_signature(&self, public_key_hex: &str) -> bool {
let key_bytes: Vec<u8> = (0..public_key_hex.len())
.step_by(2)
.filter_map(|i| u8::from_str_radix(&public_key_hex[i..i + 2], 16).ok())
.collect();
openjarvis_skills::verify_signature(&self.inner, &key_bytes)
match parse_public_key_hex(public_key_hex) {
Some(key_bytes) => openjarvis_skills::verify_signature(&self.inner, &key_bytes),
None => false,
}
}
}
fn parse_public_key_hex(public_key_hex: &str) -> Option<Vec<u8>> {
if !public_key_hex.len().is_multiple_of(2) {
return None;
}
if !public_key_hex.is_ascii() {
return None;
}
let mut key_bytes = Vec::with_capacity(public_key_hex.len() / 2);
for i in (0..public_key_hex.len()).step_by(2) {
match u8::from_str_radix(&public_key_hex[i..i + 2], 16) {
Ok(byte) => key_bytes.push(byte),
Err(_) => return None,
}
}
Some(key_bytes)
}
#[pyfunction]
pub fn load_skill(toml_str: &str) -> PyResult<PySkillManifest> {
let manifest = openjarvis_skills::load_skill(toml_str)
.map_err(|e| PyErr::new::<pyo3::exceptions::PyValueError, _>(e))?;
Ok(PySkillManifest { inner: manifest })
}
#[cfg(test)]
mod tests {
use super::parse_public_key_hex;
#[test]
fn empty_input_returns_empty_vec() {
assert_eq!(parse_public_key_hex(""), Some(Vec::new()));
}
#[test]
fn valid_hex_decodes() {
assert_eq!(
parse_public_key_hex("0a1b2cFF"),
Some(vec![0x0a, 0x1b, 0x2c, 0xff])
);
}
#[test]
fn odd_length_rejected_without_panic() {
// Regression: the pre-fix implementation sliced public_key_hex[i..i+2]
// on an odd-length string, triggering an out-of-bounds panic and a
// DoS vector when the input was attacker-controlled.
assert_eq!(parse_public_key_hex("0"), None);
assert_eq!(parse_public_key_hex("abc"), None);
assert_eq!(parse_public_key_hex("0a1b2"), None);
}
#[test]
fn non_hex_chars_rejected() {
// Pre-fix `filter_map` silently dropped non-hex chars and produced a
// truncated key, which would also have caused verification surprises.
assert_eq!(parse_public_key_hex("zz"), None);
assert_eq!(parse_public_key_hex("0aZZ"), None);
assert_eq!(parse_public_key_hex("gh"), None);
}
#[test]
fn multibyte_utf8_rejected_without_panic() {
// Pre-fix indexing public_key_hex[i..i+2] on a non-ASCII string could
// split a multi-byte UTF-8 codepoint and panic. The ASCII-only check
// makes the rejection explicit instead of relying on from_str_radix's
// post-slice error path.
assert_eq!(parse_public_key_hex("é"), None);
assert_eq!(parse_public_key_hex("aaé"), None);
}
}
+9
View File
@@ -140,6 +140,15 @@ except ImportError:
def main() -> None:
"""Entry point registered as ``jarvis`` console script."""
import sys
if sys.platform == "win32":
for _stream in (sys.stdout, sys.stderr):
if hasattr(_stream, "reconfigure"):
try:
_stream.reconfigure(encoding="utf-8", errors="replace")
except (AttributeError, OSError):
pass
cli()
+33 -3
View File
@@ -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()
@@ -535,7 +551,11 @@ def _print_profile(
"--agent",
"agent_name",
default=None,
help="Agent to use (simple, orchestrator).",
help=(
"Agent to use (simple, orchestrator, ...). "
"When omitted, falls back to ``agent.default_agent`` from config. "
"Pass ``--agent ''`` to force direct-to-engine mode (no agent)."
),
)
@click.option(
"--tools",
@@ -591,6 +611,16 @@ def ask(
# Load config
config = load_config()
# Honor `agent.default_agent` from config when --agent was not explicitly
# passed. Pass `--agent ""` to opt out and use direct-to-engine mode.
# Without this fallback, `[agent].default_system_prompt` and the
# SOUL.md / MEMORY.md / USER.md persona system are silently bypassed for
# the most common command (`jarvis ask "..."`).
if agent_name is None:
configured_default = (config.agent.default_agent or "").strip()
if configured_default:
agent_name = configured_default
# Track whether the user explicitly set --max-tokens
user_set_max_tokens = max_tokens is not None
@@ -715,8 +745,8 @@ def ask(
model_name,
)
# Agent mode
if agent_name is not None:
# Agent mode (treat empty-string `--agent ""` as explicit opt-out)
if agent_name:
parsed_tools = resolve_tool_names(
tool_names,
getattr(config.tools, "enabled", None),
+22 -1
View File
@@ -188,13 +188,34 @@ def _total_ram_gb() -> float:
if platform.system() == "Darwin":
raw = _run_cmd(["sysctl", "-n", "hw.memsize"])
return round(int(raw) / (1024**3), 1) if raw else 0.0
if platform.system() == "Windows":
import ctypes
class _MemoryStatusEx(ctypes.Structure):
_fields_ = [
("dwLength", ctypes.c_ulong),
("dwMemoryLoad", ctypes.c_ulong),
("ullTotalPhys", ctypes.c_ulonglong),
("ullAvailPhys", ctypes.c_ulonglong),
("ullTotalPageFile", ctypes.c_ulonglong),
("ullAvailPageFile", ctypes.c_ulonglong),
("ullTotalVirtual", ctypes.c_ulonglong),
("ullAvailVirtual", ctypes.c_ulonglong),
("sullAvailExtendedVirtual", ctypes.c_ulonglong),
]
stat = _MemoryStatusEx()
stat.dwLength = ctypes.sizeof(_MemoryStatusEx)
if ctypes.windll.kernel32.GlobalMemoryStatusEx(ctypes.byref(stat)):
return round(stat.ullTotalPhys / (1024**3), 1)
return 0.0
meminfo = Path("/proc/meminfo")
if meminfo.exists():
for line in meminfo.read_text().splitlines():
if line.startswith("MemTotal"):
kb = int(line.split()[1])
return round(kb / (1024**2), 1)
except (OSError, ValueError):
except (OSError, ValueError, AttributeError):
pass
return 0.0
+14
View File
@@ -88,6 +88,20 @@ class StdioTransport(MCPTransport):
raise RuntimeError("No response from subprocess")
return MCPResponse.from_json(response_line.strip())
def send_notification(self, request: MCPRequest) -> None:
"""Send a JSON-RPC notification — write only, never read.
Overrides the base implementation: stdio servers do not reply
to notifications, so the default ``send()`` would block forever
on ``proc.stdout.readline()``.
"""
proc = self._process
if proc is None or proc.stdin is None:
raise RuntimeError("Transport process is not running")
line = request.to_json() + "\n"
proc.stdin.write(line)
proc.stdin.flush()
def close(self) -> None:
"""Terminate the subprocess."""
if self._process is not None:
+4 -1
View File
@@ -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)
+134 -1
View File
@@ -212,7 +212,39 @@ class TestAskAgentOption:
)
assert result.exit_code != 0
def test_no_agent_uses_direct_mode(self, runner, mock_setup):
def test_no_agent_flag_falls_back_to_config_default_agent(
self, runner, mock_setup
):
"""When --agent is omitted, ``config.agent.default_agent`` is used.
The default ``JarvisConfig`` sets ``default_agent = "simple"``, so
``jarvis ask "..."`` should route through SimpleAgent rather than
the direct-to-engine path. Without this fallback, persona settings
(``default_system_prompt`` and SOUL.md/MEMORY.md/USER.md) would be
silently bypassed.
"""
result = runner.invoke(cli, ["ask", "Hello"])
assert result.exit_code == 0
assert "Hello from engine" in result.output
def test_explicit_empty_agent_opts_out_of_agent_mode(
self, runner, mock_setup
):
"""``--agent ""`` is the explicit opt-out: use direct-to-engine."""
result = runner.invoke(cli, ["ask", "--agent", "", "Hello"])
assert result.exit_code == 0
assert "Hello from engine" in result.output
def test_no_agent_with_blank_config_default_uses_direct_mode(
self, runner, mock_setup, monkeypatch
):
"""When config's ``default_agent`` is blank and --agent is omitted,
the original direct-to-engine path is preserved."""
from openjarvis.core.config import JarvisConfig
cfg = JarvisConfig()
cfg.agent.default_agent = ""
monkeypatch.setattr(_ask_mod, "load_config", lambda *a, **kw: cfg)
result = runner.invoke(cli, ["ask", "Hello"])
assert result.exit_code == 0
assert "Hello from engine" in result.output
@@ -304,3 +336,104 @@ 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
+9
View File
@@ -32,6 +32,15 @@ def _mock_engine_response():
def _patch_ask(monkeypatch, tmp_path, *, engine_result=None, no_engine=False):
"""Set up common mocks for ask tests."""
# Re-register SimpleAgent after the autouse `_clean_registries` conftest
# fixture clears it. ``JarvisConfig().agent.default_agent`` defaults to
# ``"simple"``, so ``jarvis ask "..."`` (no --agent) routes through it.
from openjarvis.agents.simple import SimpleAgent
from openjarvis.core.registry import AgentRegistry
if not AgentRegistry.contains("simple"):
AgentRegistry.register_value("simple", SimpleAgent)
cfg = JarvisConfig()
cfg.telemetry.db_path = str(tmp_path / "telemetry.db")
+17
View File
@@ -27,8 +27,23 @@ def _mock_engine():
return engine
def _register_agents():
"""Re-register agents after the conftest registry clear.
The default ``JarvisConfig().agent.default_agent`` is ``"simple"``,
so ``jarvis ask "..."`` (without ``--agent``) routes through SimpleAgent.
Without this re-registration, that path raises ``Unknown agent: simple``.
"""
from openjarvis.agents.simple import SimpleAgent
from openjarvis.core.registry import AgentRegistry
if not AgentRegistry.contains("simple"):
AgentRegistry.register_value("simple", SimpleAgent)
def _patch_engine(engine):
"""Return context managers that patch engine discovery to use our mock."""
_register_agents()
return (
mock.patch.object(
_ask_mod,
@@ -96,6 +111,7 @@ class TestAskModelResolution:
cfg.intelligence.temperature = 0.7
cfg.intelligence.max_tokens = 1024
cfg.agent.context_from_memory = False
cfg.agent.default_agent = ""
result = CliRunner().invoke(cli, ["ask", "Hello"])
assert result.exit_code == 0
@@ -127,5 +143,6 @@ class TestAskModelResolution:
cfg.intelligence.temperature = 0.7
cfg.intelligence.max_tokens = 1024
cfg.agent.context_from_memory = False
cfg.agent.default_agent = ""
result = CliRunner().invoke(cli, ["ask", "Hello"])
assert result.exit_code == 0
+40 -1
View File
@@ -2,13 +2,52 @@
from __future__ import annotations
import io
import sys
from pathlib import Path
from unittest import mock
from click.testing import CliRunner
import openjarvis
from openjarvis.cli import cli
from openjarvis.cli import cli, main
class TestMainEntryPoint:
"""Tests for the ``jarvis`` console script entry point."""
def test_windows_reconfigures_stdout_to_utf8(self) -> None:
"""On Windows, main() must reconfigure stdout/stderr to UTF-8 so that
CJK characters in CLI output don't trigger UnicodeEncodeError under
legacy code pages (cp950, cp932, cp949)."""
stdout_mock = mock.MagicMock(spec=io.TextIOWrapper)
stderr_mock = mock.MagicMock(spec=io.TextIOWrapper)
with (
mock.patch.object(sys, "platform", "win32"),
mock.patch.object(sys, "stdout", stdout_mock),
mock.patch.object(sys, "stderr", stderr_mock),
mock.patch("openjarvis.cli.cli") as cli_mock,
):
main()
stdout_mock.reconfigure.assert_called_once_with(
encoding="utf-8", errors="replace"
)
stderr_mock.reconfigure.assert_called_once_with(
encoding="utf-8", errors="replace"
)
cli_mock.assert_called_once()
def test_non_windows_does_not_reconfigure(self) -> None:
"""On non-Windows platforms, stdout/stderr are left untouched."""
stdout_mock = mock.MagicMock(spec=io.TextIOWrapper)
with (
mock.patch.object(sys, "platform", "linux"),
mock.patch.object(sys, "stdout", stdout_mock),
mock.patch("openjarvis.cli.cli") as cli_mock,
):
main()
stdout_mock.reconfigure.assert_not_called()
cli_mock.assert_called_once()
class TestCLI:
+29
View File
@@ -3,13 +3,17 @@ and engine recommendation."""
from __future__ import annotations
import sys
from unittest.mock import patch
import pytest
from openjarvis.core.config import (
GpuInfo,
_detect_amd_gpu,
_detect_apple_gpu,
_detect_nvidia_gpu,
_total_ram_gb,
recommend_engine,
)
@@ -75,6 +79,31 @@ class TestDetectHardware:
assert _detect_apple_gpu() is None
# ---------------------------------------------------------------------------
# RAM detection
# ---------------------------------------------------------------------------
class TestTotalRamGb:
"""Tests for _total_ram_gb() across platforms."""
@pytest.mark.skipif(sys.platform != "win32", reason="Requires Windows")
def test_total_ram_gb_windows(self):
"""On Windows, GlobalMemoryStatusEx must return a positive RAM value."""
ram = _total_ram_gb()
assert ram > 0, f"Expected > 0 GB on Windows, got {ram}"
@pytest.mark.skipif(sys.platform != "darwin", reason="Requires macOS")
def test_total_ram_gb_darwin(self):
ram = _total_ram_gb()
assert ram > 0, f"Expected > 0 GB on macOS, got {ram}"
@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="Requires Linux")
def test_total_ram_gb_linux(self):
ram = _total_ram_gb()
assert ram > 0, f"Expected > 0 GB on Linux, got {ram}"
# ---------------------------------------------------------------------------
# Engine recommendation
# ---------------------------------------------------------------------------
+48
View File
@@ -136,6 +136,54 @@ class TestStdioTransport:
finally:
transport.close()
def test_send_notification_does_not_read_stdout(self, tmp_path):
"""Regression for #339: stdio servers don't reply to notifications.
The base ``MCPTransport.send_notification`` falls back to ``send()``,
which writes the request and then blocks on ``proc.stdout.readline()``.
For stdio MCP servers that never reply to a notification, that read
hangs forever, breaking the JSON-RPC ``notifications/initialized``
handshake. ``StdioTransport.send_notification`` must override that
behavior to be write-only.
This test spawns a subprocess that consumes stdin without ever
writing to stdout, then issues ``send_notification`` from a worker
thread. If the override is missing, the thread blocks on
``readline()`` and the join timeout fires.
"""
import threading
script = tmp_path / "silent_consumer.py"
script.write_text(
textwrap.dedent("""\
import sys
for _ in sys.stdin:
pass
""")
)
transport = StdioTransport([sys.executable, str(script)])
try:
notification = MCPRequest(method="notifications/initialized")
result_box: dict = {}
def call():
try:
transport.send_notification(notification)
result_box["ok"] = True
except Exception as exc: # noqa: BLE001
result_box["error"] = exc
worker = threading.Thread(target=call, daemon=True)
worker.start()
worker.join(timeout=2.0)
assert not worker.is_alive(), (
"send_notification blocked on stdout — override missing"
)
assert result_box.get("ok") is True, result_box
finally:
transport.close()
def test_close_terminates_process(self, tmp_path):
script = tmp_path / "sleep_server.py"
script.write_text(
+5
View File
@@ -88,6 +88,11 @@ def _energy_config(tmp_path, gpu_metrics=True):
cfg.telemetry.gpu_metrics = gpu_metrics
cfg.telemetry.energy_vendor = ""
cfg.telemetry.db_path = str(tmp_path / "telemetry.db")
# These tests exercise engine-level instrumentation, not agent dispatch.
# `jarvis ask` (no --agent) now falls back to ``agent.default_agent``
# which defaults to "simple", and conftest clears the registry per test.
# Opt out explicitly so the CLI uses direct-engine mode here.
cfg.agent.default_agent = ""
return cfg