diff --git a/rust/crates/openjarvis-python/src/skills.rs b/rust/crates/openjarvis-python/src/skills.rs index b228c9e2..940c43d7 100644 --- a/rust/crates/openjarvis-python/src/skills.rs +++ b/rust/crates/openjarvis-python/src/skills.rs @@ -48,17 +48,81 @@ impl PySkillManifest { } fn verify_signature(&self, public_key_hex: &str) -> bool { - let key_bytes: Vec = (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> { + 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 { let manifest = openjarvis_skills::load_skill(toml_str) .map_err(|e| PyErr::new::(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); + } +} diff --git a/src/openjarvis/cli/__init__.py b/src/openjarvis/cli/__init__.py index 82df6998..f42773a4 100644 --- a/src/openjarvis/cli/__init__.py +++ b/src/openjarvis/cli/__init__.py @@ -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() diff --git a/src/openjarvis/cli/ask.py b/src/openjarvis/cli/ask.py index b1460ae1..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() @@ -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), diff --git a/src/openjarvis/core/config.py b/src/openjarvis/core/config.py index 2eddc7a6..a00f48cd 100644 --- a/src/openjarvis/core/config.py +++ b/src/openjarvis/core/config.py @@ -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 diff --git a/src/openjarvis/mcp/transport.py b/src/openjarvis/mcp/transport.py index 2c40bacc..11f8020f 100644 --- a/src/openjarvis/mcp/transport.py +++ b/src/openjarvis/mcp/transport.py @@ -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: 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 622c2910..d18ac5aa 100644 --- a/tests/cli/test_ask_agent.py +++ b/tests/cli/test_ask_agent.py @@ -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 diff --git a/tests/cli/test_ask_e2e.py b/tests/cli/test_ask_e2e.py index 336f58b4..3f833d14 100644 --- a/tests/cli/test_ask_e2e.py +++ b/tests/cli/test_ask_e2e.py @@ -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") diff --git a/tests/cli/test_ask_router.py b/tests/cli/test_ask_router.py index a4f5fbbe..ddbae1e4 100644 --- a/tests/cli/test_ask_router.py +++ b/tests/cli/test_ask_router.py @@ -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 diff --git a/tests/cli/test_cli.py b/tests/cli/test_cli.py index 558741c5..63668b36 100644 --- a/tests/cli/test_cli.py +++ b/tests/cli/test_cli.py @@ -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: diff --git a/tests/hardware/test_hardware_profiles.py b/tests/hardware/test_hardware_profiles.py index d1686931..d35740a8 100644 --- a/tests/hardware/test_hardware_profiles.py +++ b/tests/hardware/test_hardware_profiles.py @@ -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 # --------------------------------------------------------------------------- diff --git a/tests/mcp/test_transport.py b/tests/mcp/test_transport.py index 4cd10e86..17ec4bb5 100644 --- a/tests/mcp/test_transport.py +++ b/tests/mcp/test_transport.py @@ -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( diff --git a/tests/telemetry/test_energy_wiring.py b/tests/telemetry/test_energy_wiring.py index 399b717d..06ec1cb6 100644 --- a/tests/telemetry/test_energy_wiring.py +++ b/tests/telemetry/test_energy_wiring.py @@ -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