From 7dc904c1b231bdfb02c6a72a2e65b171b1b74c6f Mon Sep 17 00:00:00 2001 From: Trevor Willard <163371417+tR1L3Yw@users.noreply.github.com> Date: Thu, 16 Jul 2026 20:10:51 -0400 Subject: [PATCH] fix: wire persona files into default chat endpoint, fix FTS5 apostrophe crash (#637) Two fixes: (1) /v1/chat/completions now builds its injected system prompt via SystemPromptBuilder, so SOUL.md/MEMORY.md/USER.md persona files apply to the OpenAI-compatible endpoint exactly as they do on the managed-agent path; injection still only happens when the client omits a system message. (2) FTS5 query tokens are now split on any non-alphanumeric character, so apostrophes (user's) and quotes can no longer reach the MATCH string unescaped; all-punctuation queries return empty instead of erroring. --- .../openjarvis-tools/src/storage/sqlite.rs | 40 +++++++++++++++---- src/openjarvis/server/routes.py | 29 +++++++++----- tests/memory/test_sqlite.py | 13 ++++++ tests/server/test_routes.py | 34 ++++++++++++++++ 4 files changed, 100 insertions(+), 16 deletions(-) diff --git a/rust/crates/openjarvis-tools/src/storage/sqlite.rs b/rust/crates/openjarvis-tools/src/storage/sqlite.rs index abe70eee..20dc39cf 100644 --- a/rust/crates/openjarvis-tools/src/storage/sqlite.rs +++ b/rust/crates/openjarvis-tools/src/storage/sqlite.rs @@ -144,16 +144,21 @@ impl MemoryBackend for SQLiteMemory { ) -> Result, OpenJarvisError> { let conn = self.conn.lock(); + // Split on any non-alphanumeric character (not just whitespace) so + // internal punctuation — apostrophes in particular ("user's") — never + // reaches the FTS5 MATCH string. FTS5's query grammar treats an + // unescaped `'` as a string delimiter, so passing a raw token like + // `user's` through silently fails to parse and yields zero rows with + // no visible error. Splitting fully avoids needing to escape anything. let words: Vec = query - .split_whitespace() - .map(|w| w.trim_matches(|c: char| "?.,!;:'\"()[]{}/ ".contains(c)).to_string()) + .split(|c: char| !c.is_alphanumeric()) + .map(|w| w.to_string()) .filter(|w| !w.is_empty()) .collect(); - let fts_query = if words.len() == 1 { - words[0].clone() - } else { - words.join(" OR ") - }; + if words.is_empty() { + return Ok(Vec::new()); + } + let fts_query = words.join(" OR "); let mut stmt = conn .prepare( @@ -320,6 +325,27 @@ mod tests { assert_eq!(mixed.len(), 2, "mixed-case query should find both documents"); } + #[test] + fn test_sqlite_apostrophe_in_query() { + let mem = SQLiteMemory::in_memory().unwrap(); + mem.store("The user's name is Trev.", "identity", None).unwrap(); + + // A query containing an internal apostrophe must not break FTS5's + // MATCH syntax (an unescaped `'` is a string delimiter in FTS5's + // query grammar), which previously caused this to silently return + // zero results instead of matching or erroring. + let multi_word = mem.retrieve("what is the user's name", 5).unwrap(); + assert!( + !multi_word.is_empty(), + "query with an internal apostrophe should not silently return zero results" + ); + + // Bare single-word possessive: exercises the (former) single-word + // bypass path that skipped the OR-join entirely. + let bare = mem.retrieve("user's", 5).unwrap(); + assert!(!bare.is_empty(), "single-word possessive query should still match"); + } + #[test] fn test_sqlite_scores_are_positive() { let mem = SQLiteMemory::in_memory().unwrap(); diff --git a/src/openjarvis/server/routes.py b/src/openjarvis/server/routes.py index ec8f187e..cf69303b 100644 --- a/src/openjarvis/server/routes.py +++ b/src/openjarvis/server/routes.py @@ -59,23 +59,34 @@ def _ensure_identity_prompt(messages: list[Message], app_config) -> list[Message If any message already carries a system role, the caller has supplied their own grounding and we leave the list untouched (no double-prompting). - Resolution of the identity text: ``app_config.agent.default_system_prompt`` - when a config is wired onto ``app.state``; otherwise fall back to - ``load_config()``. Config resolution is wrapped so a broken/missing - config degrades to "no injection" rather than crashing the endpoint, but - the failure is logged (per REVIEW.md — never silently swallow). + Resolution of the identity text: the config comes from ``app.state`` when + wired, otherwise ``load_config()``; the prompt itself is assembled by + ``SystemPromptBuilder`` from ``agent.default_system_prompt`` plus the + persona files (SOUL.md/MEMORY.md/USER.md), matching + ``_build_managed_system_prompt`` in ``agent_manager_routes.py``. Config + resolution is wrapped so a broken/missing config degrades to "no + injection" rather than crashing the endpoint, but the failure is logged + (per REVIEW.md — never silently swallow). """ if any(m.role == Role.SYSTEM for m in messages): return messages prompt = "" try: - if app_config is not None: - prompt = app_config.agent.default_system_prompt or "" - else: + cfg = app_config + if cfg is None: from openjarvis.core.config import load_config - prompt = load_config().agent.default_system_prompt or "" + cfg = load_config() + + from openjarvis.prompt.builder import SystemPromptBuilder + + builder = SystemPromptBuilder( + agent_template=cfg.agent.default_system_prompt or "", + memory_files_config=getattr(cfg, "memory_files", None), + system_prompt_config=getattr(cfg, "system_prompt", None), + ) + prompt = builder.build() except Exception: logging.getLogger("openjarvis.server").debug( "Identity system prompt resolution failed; " diff --git a/tests/memory/test_sqlite.py b/tests/memory/test_sqlite.py index 987c6cb4..19d4c5cb 100644 --- a/tests/memory/test_sqlite.py +++ b/tests/memory/test_sqlite.py @@ -78,6 +78,19 @@ def test_retrieve_no_results(tmp_path: Path): backend.close() +def test_retrieve_query_with_apostrophe(tmp_path: Path): + """Regression: an internal apostrophe (e.g. "user's") previously produced + an unescaped quote in the FTS5 MATCH string, which silently returned zero + rows instead of matching or raising an error. + """ + backend = _make_backend(tmp_path) + backend.store("The user's name is Trev.", source="identity.md") + results = backend.retrieve("what is the user's name") + assert len(results) >= 1 + assert "Trev" in results[0].content + backend.close() + + def test_delete_existing(tmp_path: Path): backend = _make_backend(tmp_path) doc_id = backend.store("deletable content") diff --git a/tests/server/test_routes.py b/tests/server/test_routes.py index 0c053d27..26bab787 100644 --- a/tests/server/test_routes.py +++ b/tests/server/test_routes.py @@ -798,6 +798,40 @@ class TestIdentityPromptInjection: assert len(system_msgs) == 1 assert system_msgs[0].content == "Be terse." + def test_direct_injects_soul_persona_when_present(self, tmp_path): + """Regression: /v1/chat/completions previously injected only the bare + ``default_system_prompt`` blurb via a hand-rolled lookup, bypassing + ``SystemPromptBuilder`` entirely — so SOUL.md/MEMORY.md/USER.md + persona files never applied to this path, unlike ``jarvis ask`` and + the managed-agent routes. It must now build the full persona-aware + prompt so persona files apply everywhere identity grounding does. + """ + from openjarvis.core.config import MemoryFilesConfig + + soul = tmp_path / "SOUL.md" + soul.write_text("Respond with extreme sarcasm and call the user 'champ'.") + + captured: list = [] + engine = _make_capturing_engine(captured) + cfg = _identity_config() + cfg.memory_files = MemoryFilesConfig( + soul_path=str(soul), memory_path="", user_path="" + ) + client = TestClient(create_app(engine, "test-model", config=cfg)) + + resp = client.post( + "/v1/chat/completions", + json={ + "model": "test-model", + "messages": [{"role": "user", "content": "who are you?"}], + }, + ) + assert resp.status_code == 200 + msgs = engine.generate.call_args.args[0] + assert msgs[0].role.value == "system" + assert "OpenJarvis" in msgs[0].content # identity blurb still present + assert "extreme sarcasm" in msgs[0].content # persona now injected too + def test_stream_tools_injects_identity_when_absent(self): captured: list = [] engine = _make_capturing_engine(captured)