mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-08-14 00:47:52 +00:00
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.
This commit is contained in:
@@ -144,16 +144,21 @@ impl MemoryBackend for SQLiteMemory {
|
|||||||
) -> Result<Vec<RetrievalResult>, OpenJarvisError> {
|
) -> Result<Vec<RetrievalResult>, OpenJarvisError> {
|
||||||
let conn = self.conn.lock();
|
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<String> = query
|
let words: Vec<String> = query
|
||||||
.split_whitespace()
|
.split(|c: char| !c.is_alphanumeric())
|
||||||
.map(|w| w.trim_matches(|c: char| "?.,!;:'\"()[]{}/ ".contains(c)).to_string())
|
.map(|w| w.to_string())
|
||||||
.filter(|w| !w.is_empty())
|
.filter(|w| !w.is_empty())
|
||||||
.collect();
|
.collect();
|
||||||
let fts_query = if words.len() == 1 {
|
if words.is_empty() {
|
||||||
words[0].clone()
|
return Ok(Vec::new());
|
||||||
} else {
|
}
|
||||||
words.join(" OR ")
|
let fts_query = words.join(" OR ");
|
||||||
};
|
|
||||||
|
|
||||||
let mut stmt = conn
|
let mut stmt = conn
|
||||||
.prepare(
|
.prepare(
|
||||||
@@ -320,6 +325,27 @@ mod tests {
|
|||||||
assert_eq!(mixed.len(), 2, "mixed-case query should find both documents");
|
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]
|
#[test]
|
||||||
fn test_sqlite_scores_are_positive() {
|
fn test_sqlite_scores_are_positive() {
|
||||||
let mem = SQLiteMemory::in_memory().unwrap();
|
let mem = SQLiteMemory::in_memory().unwrap();
|
||||||
|
|||||||
@@ -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
|
If any message already carries a system role, the caller has supplied
|
||||||
their own grounding and we leave the list untouched (no double-prompting).
|
their own grounding and we leave the list untouched (no double-prompting).
|
||||||
|
|
||||||
Resolution of the identity text: ``app_config.agent.default_system_prompt``
|
Resolution of the identity text: the config comes from ``app.state`` when
|
||||||
when a config is wired onto ``app.state``; otherwise fall back to
|
wired, otherwise ``load_config()``; the prompt itself is assembled by
|
||||||
``load_config()``. Config resolution is wrapped so a broken/missing
|
``SystemPromptBuilder`` from ``agent.default_system_prompt`` plus the
|
||||||
config degrades to "no injection" rather than crashing the endpoint, but
|
persona files (SOUL.md/MEMORY.md/USER.md), matching
|
||||||
the failure is logged (per REVIEW.md — never silently swallow).
|
``_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):
|
if any(m.role == Role.SYSTEM for m in messages):
|
||||||
return messages
|
return messages
|
||||||
|
|
||||||
prompt = ""
|
prompt = ""
|
||||||
try:
|
try:
|
||||||
if app_config is not None:
|
cfg = app_config
|
||||||
prompt = app_config.agent.default_system_prompt or ""
|
if cfg is None:
|
||||||
else:
|
|
||||||
from openjarvis.core.config import load_config
|
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:
|
except Exception:
|
||||||
logging.getLogger("openjarvis.server").debug(
|
logging.getLogger("openjarvis.server").debug(
|
||||||
"Identity system prompt resolution failed; "
|
"Identity system prompt resolution failed; "
|
||||||
|
|||||||
@@ -78,6 +78,19 @@ def test_retrieve_no_results(tmp_path: Path):
|
|||||||
backend.close()
|
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):
|
def test_delete_existing(tmp_path: Path):
|
||||||
backend = _make_backend(tmp_path)
|
backend = _make_backend(tmp_path)
|
||||||
doc_id = backend.store("deletable content")
|
doc_id = backend.store("deletable content")
|
||||||
|
|||||||
@@ -798,6 +798,40 @@ class TestIdentityPromptInjection:
|
|||||||
assert len(system_msgs) == 1
|
assert len(system_msgs) == 1
|
||||||
assert system_msgs[0].content == "Be terse."
|
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):
|
def test_stream_tools_injects_identity_when_absent(self):
|
||||||
captured: list = []
|
captured: list = []
|
||||||
engine = _make_capturing_engine(captured)
|
engine = _make_capturing_engine(captured)
|
||||||
|
|||||||
Reference in New Issue
Block a user