From f21eec6c862970d8f031fecfd6cefe5f0cd87f10 Mon Sep 17 00:00:00 2001 From: tomaioo Date: Sat, 11 Apr 2026 07:15:05 +0700 Subject: [PATCH 01/10] fix(security): panic on malformed hex input in signature verifica `verify_signature` slices `public_key_hex[i..i + 2]` without validating that the input length is even. An odd-length or otherwise malformed string can trigger an out-of-bounds panic, which may crash the process (or at minimum terminate the request path), creating a denial-of-service vector if this method is reachable from untrusted input. Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com> --- rust/crates/openjarvis-python/src/skills.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/rust/crates/openjarvis-python/src/skills.rs b/rust/crates/openjarvis-python/src/skills.rs index b228c9e2..7f9cea4a 100644 --- a/rust/crates/openjarvis-python/src/skills.rs +++ b/rust/crates/openjarvis-python/src/skills.rs @@ -48,10 +48,18 @@ 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(); + if public_key_hex.len() % 2 != 0 { + return false; + } + + 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 false, + } + } + openjarvis_skills::verify_signature(&self.inner, &key_bytes) } } From 4177b5c50e3e083976e2a918157cbd165ca60d53 Mon Sep 17 00:00:00 2001 From: krypticmouse Date: Wed, 20 May 2026 03:36:28 +0000 Subject: [PATCH 02/10] fix(skills): satisfy Clippy + reject non-ASCII hex + add regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on @tomaioo's signature-verification panic fix in PR #235. 1. Clippy on Rust 1.95+ flags `len() % 2 != 0` with the `manual_is_multiple_of` lint, which was failing the `rust` CI job on PR #235 and blocking merge. Switch to `is_multiple_of(2)`. 2. Add an explicit `is_ascii()` guard before slicing. With only the length check, a non-ASCII input (e.g. `"é"` — 2 bytes but 1 char) would survive the length check before `from_str_radix` caught it. The explicit guard makes the rejection intent clear and avoids relying on the post-slice error path. 3. Extract the hex-parsing into a private `parse_public_key_hex` helper. PyO3-bound `#[pymethods]` are awkward to unit-test from Rust (need a Python interpreter via `prepare_freethreaded_python`); a plain function is testable with no GIL boilerplate. 4. Add 5 unit tests covering the security boundary: - empty input -> Some(empty vec) - valid hex decodes to the right bytes - odd-length rejected without panic (the original bug) - non-hex chars rejected (previously silently filtered) - multi-byte UTF-8 rejected without panic Co-Authored-By: Claude Opus 4.7 (1M context) --- rust/crates/openjarvis-python/src/skills.rs | 80 +++++++++++++++++---- 1 file changed, 68 insertions(+), 12 deletions(-) diff --git a/rust/crates/openjarvis-python/src/skills.rs b/rust/crates/openjarvis-python/src/skills.rs index 7f9cea4a..940c43d7 100644 --- a/rust/crates/openjarvis-python/src/skills.rs +++ b/rust/crates/openjarvis-python/src/skills.rs @@ -48,25 +48,81 @@ impl PySkillManifest { } fn verify_signature(&self, public_key_hex: &str) -> bool { - if public_key_hex.len() % 2 != 0 { - return false; + match parse_public_key_hex(public_key_hex) { + Some(key_bytes) => openjarvis_skills::verify_signature(&self.inner, &key_bytes), + None => false, } - - 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 false, - } - } - - openjarvis_skills::verify_signature(&self.inner, &key_bytes) } } +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); + } +} From df566d4186eaa70ace040e0214242c3ec74db516 Mon Sep 17 00:00:00 2001 From: Gilles Ceyssat Date: Tue, 12 May 2026 07:26:33 +0400 Subject: [PATCH 03/10] fix(mcp): StdioTransport.send_notification must not read response The base MCPTransport.send_notification falls back to self.send() which writes a request AND reads a response. For stdio MCP servers this hangs forever because notifications (per JSON-RPC 2.0 spec) have no response. This affected every stdio MCP server attached to OpenJarvis: after MCPClient.initialize() sent its `initialize` request and received the server capabilities, it sent the spec-required `notifications/initialized` notification, which then blocked indefinitely on proc.stdout.readline(). StreamableHTTPTransport already overrides send_notification correctly (transport.py:213). This adds the symmetric fix for StdioTransport so stdio MCP servers can complete the handshake and be discovered. Reproduced with the CyberStrikeAI cmd/mcp-stdio server (78 tools): without the fix, `_discover_external_mcp` hangs forever in `MCPClient.initialize`. With the fix, all 78 tools are discovered cleanly. --- src/openjarvis/mcp/transport.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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: From 8b5a30422268ed58e7a4dcb6ec4e513758db6b2d Mon Sep 17 00:00:00 2001 From: krypticmouse Date: Wed, 20 May 2026 03:36:37 +0000 Subject: [PATCH 04/10] test(mcp): regression test for StdioTransport.send_notification Follow-up on @Dilligaf371's PR #339 fix. Adds a regression test that spawns a subprocess which consumes stdin without ever writing to stdout, then issues `send_notification` from a worker thread. If the override is missing, the thread blocks forever on `proc.stdout.readline()` and the 2-second join timeout fires. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/mcp/test_transport.py | 48 +++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) 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( From 31efd9e672cc76f5e8dd9094880a3327b6975c7c Mon Sep 17 00:00:00 2001 From: IsaacH Date: Thu, 30 Apr 2026 11:06:29 +0800 Subject: [PATCH 05/10] fix: detect total RAM on Windows via GlobalMemoryStatusEx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_total_ram_gb()` had branches for Darwin (sysctl) and Linux (/proc/meminfo) but no Windows path, so `jarvis init` reported "0.0 GB RAM" on every Windows host. The downstream `recommend_model()` then fell back to VRAM-only sizing, often selecting a smaller tier than the system can actually run. Add a Windows branch that calls `GlobalMemoryStatusEx` via ctypes — no new dependency. Add a platform-skip-aware test for each OS branch. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/openjarvis/core/config.py | 23 ++++++++++++++++++- tests/hardware/test_hardware_profiles.py | 29 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/openjarvis/core/config.py b/src/openjarvis/core/config.py index 10aafd22..7458f6f8 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/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 # --------------------------------------------------------------------------- From 30ac635e831e81000deadd0085c6dfdd15316f56 Mon Sep 17 00:00:00 2001 From: IsaacH Date: Thu, 30 Apr 2026 11:06:36 +0800 Subject: [PATCH 06/10] fix: force UTF-8 stdout on Windows for CJK CLI output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows the default Python stdout encoding follows the system ANSI code page (cp950 for zh-TW, cp932 for ja, cp949 for ko). `click.echo()` then raises `UnicodeEncodeError` whenever a CJK character lands in CLI output — `jarvis ask` returning Chinese crashes with `'cp950' codec can't encode character '义'`. Reconfigure `sys.stdout` and `sys.stderr` to UTF-8 with `errors='replace'` at the `main()` entry point. Scoped to `win32` so other platforms are untouched. Two unit tests verify the reconfigure happens on Windows and doesn't on Linux. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/openjarvis/cli/__init__.py | 9 ++++++++ tests/cli/test_cli.py | 41 +++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) 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/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: From c7f451fbc896a0e3ca6cf8c3f860276898c7b7ab Mon Sep 17 00:00:00 2001 From: IsaacH Date: Thu, 30 Apr 2026 11:47:40 +0800 Subject: [PATCH 07/10] fix(ask): honor agent.default_agent from config when --agent omitted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `jarvis ask "..."` (no `--agent` flag) routed straight to `engine.generate()` regardless of `agent.default_agent` in the user's config. As a result the persona stack — `agent.default_system_prompt`, SOUL.md, MEMORY.md, USER.md — was silently bypassed for the most common command. Behavior now: - `--agent X` → use agent X (unchanged) - `--agent ""` (empty) → explicit opt-out, direct-to-engine mode - (omitted) → fall back to `config.agent.default_agent`, which dataclass-defaults to `"simple"`, so persona settings finally take effect Tests: - Rename `test_no_agent_uses_direct_mode` to `test_no_agent_flag_falls_back_to_config_default_agent` and update its docstring to document the new behavior. - Add `test_explicit_empty_agent_opts_out_of_agent_mode`. - Add `test_no_agent_with_blank_config_default_uses_direct_mode` to cover the case where the user clears `default_agent`. - Update `_patch_engine` (test_ask_router) and `_patch_ask` (test_ask_e2e) to re-register `SimpleAgent` after the autouse `_clean_registries` conftest fixture, since the agent path now runs in tests that previously short-circuited to direct mode. - Add explicit `cfg.agent.default_agent = ""` to two `test_ask_router` tests that mock load_config with a MagicMock (so `cfg.agent.default_agent` doesn't auto-create as a truthy mock). Note: `tests/cli/test_ask_context.py` has 2 unrelated pre-existing failures on origin/main (memory backend returns None on Windows); those are out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/openjarvis/cli/ask.py | 20 +++++++++++++++++--- tests/cli/test_ask_agent.py | 34 +++++++++++++++++++++++++++++++++- tests/cli/test_ask_e2e.py | 9 +++++++++ tests/cli/test_ask_router.py | 17 +++++++++++++++++ 4 files changed, 76 insertions(+), 4 deletions(-) diff --git a/src/openjarvis/cli/ask.py b/src/openjarvis/cli/ask.py index b1460ae1..165d5f68 100644 --- a/src/openjarvis/cli/ask.py +++ b/src/openjarvis/cli/ask.py @@ -535,7 +535,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 +595,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 +729,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/tests/cli/test_ask_agent.py b/tests/cli/test_ask_agent.py index 622c2910..8da0e234 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 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 From 0217a901ddbae2bc95156120bb541d318843493c Mon Sep 17 00:00:00 2001 From: krypticmouse Date: Wed, 20 May 2026 03:37:09 +0000 Subject: [PATCH 08/10] test(energy_wiring): opt out of #294 default-agent fallback in _energy_config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on @TX-Huang's PR #294. The default-agent fallback now routes `jarvis ask "..."` (no --agent) through `config.agent.default_agent`, which dataclass-defaults to `"simple"`. The autouse `_clean_registries` fixture in tests/conftest.py clears AgentRegistry between tests, so the fallback then fails with `Unknown agent: simple` and every CLI-driven test_energy_wiring test exits with code 1. These tests exercise engine-level instrumentation, not agent dispatch — ``test_engine_wrapped_with_instrumented``, the energy-monitor lifecycle tests, and the end-to-end pipeline tests all care that the engine gets wrapped and telemetry lands in SQLite, regardless of whether the call goes through an agent. Set ``cfg.agent.default_agent = ""`` in ``_energy_config`` to keep these tests on the direct-engine path they were originally designed for. The dedicated ``test_agent_mode_uses_instrumented_engine`` (which explicitly passes ``--agent``) is unaffected. Same pattern PR #294 already uses in tests/cli/test_ask_router.py for the two MagicMock-config tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/telemetry/test_energy_wiring.py | 5 +++++ 1 file changed, 5 insertions(+) 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 From 90f009e9062886197b4db1775e34c19cb3a85490 Mon Sep 17 00:00:00 2001 From: IsaacH Date: Thu, 30 Apr 2026 11:53:22 +0800 Subject: [PATCH 09/10] feat(ask): wire SystemPromptBuilder so SOUL.md / MEMORY.md / USER.md actually load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 ` 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) --- src/openjarvis/cli/ask.py | 16 ++++++ src/openjarvis/prompt/builder.py | 5 +- tests/cli/test_ask_agent.py | 97 ++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/openjarvis/cli/ask.py b/src/openjarvis/cli/ask.py index 165d5f68..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() 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 8da0e234..a1c4358f 100644 --- a/tests/cli/test_ask_agent.py +++ b/tests/cli/test_ask_agent.py @@ -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 From 9a42e62172b288cabb16f6a57bb5cd11a6552aeb Mon Sep 17 00:00:00 2001 From: krypticmouse Date: Wed, 20 May 2026 03:37:18 +0000 Subject: [PATCH 10/10] style: wrap long line in test_ask_agent.py Follow-up on @TX-Huang's PR #295. The new test_soul_md_content_reaches_engine_in_simple_agent test has one line at 92 chars that trips ruff's E501 (88 char limit). Wrap the ternary onto multiple lines so the file passes ruff check on CI. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/cli/test_ask_agent.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/cli/test_ask_agent.py b/tests/cli/test_ask_agent.py index a1c4358f..d18ac5aa 100644 --- a/tests/cli/test_ask_agent.py +++ b/tests/cli/test_ask_agent.py @@ -390,7 +390,11 @@ class TestPersonaFilesReachModel: # 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") + 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}"