diff --git a/agent/memory_manager.py b/agent/memory_manager.py index 240595a4eb3..dcd50a2997a 100644 --- a/agent/memory_manager.py +++ b/agent/memory_manager.py @@ -33,6 +33,7 @@ from concurrent.futures import ThreadPoolExecutor from typing import Any, Dict, List, Optional from agent.memory_provider import MemoryProvider +from agent.skill_commands import extract_user_instruction_from_skill_message from tools.registry import tool_error logger = logging.getLogger(__name__) @@ -430,16 +431,37 @@ class MemoryManager: # -- Prefetch / recall --------------------------------------------------- + @staticmethod + def _strip_skill_scaffolding(text: str) -> Optional[str]: + """Return memory-worthy user text, or None to skip the turn. + + When a user invokes a /skill or /bundle, Hermes expands the turn into + a model-facing message that embeds the entire skill body. Feeding that + verbatim to memory providers pollutes their stores/embeddings with + prompt scaffolding instead of what the user actually asked. We recover + just the user's instruction here, once, for every provider — so this + is fixed for the whole provider fan-out, not per backend. + + - Non-skill messages pass through unchanged. + - Skill turns with a user instruction return that instruction. + - Bare skill invocations (no instruction) return None → callers skip + the turn, since there is no user content worth remembering. + """ + return extract_user_instruction_from_skill_message(text) + def prefetch_all(self, query: str, *, session_id: str = "") -> str: """Collect prefetch context from all providers. Returns merged context text labeled by provider. Empty providers are skipped. Failures in one provider don't block others. """ + clean_query = self._strip_skill_scaffolding(query) + if not clean_query: + return "" parts = [] for provider in self._providers: try: - result = provider.prefetch(query, session_id=session_id) + result = provider.prefetch(clean_query, session_id=session_id) if result and result.strip(): parts.append(result) except Exception as e: @@ -460,10 +482,14 @@ class MemoryManager: if not providers: return + clean_query = self._strip_skill_scaffolding(query) + if not clean_query: + return + def _run() -> None: for provider in providers: try: - provider.queue_prefetch(query, session_id=session_id) + provider.queue_prefetch(clean_query, session_id=session_id) except Exception as e: logger.debug( "Memory provider '%s' queue_prefetch failed (non-fatal): %s", @@ -515,6 +541,11 @@ class MemoryManager: if not providers: return + clean_user_content = self._strip_skill_scaffolding(user_content) + if not clean_user_content: + return + user_content = clean_user_content + def _run() -> None: for provider in providers: try: diff --git a/agent/skill_commands.py b/agent/skill_commands.py index 269c2fdd25e..18264c44bd3 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -26,6 +26,91 @@ _skill_commands_platform: Optional[str] = None _SKILL_INVALID_CHARS = re.compile(r"[^a-z0-9-]") _SKILL_MULTI_HYPHEN = re.compile(r"-{2,}") +# --------------------------------------------------------------------------- +# Skill-scaffolding markers and the canonical extractor. +# +# When a user invokes a /skill (or /bundle), Hermes expands the turn into a +# model-facing message that embeds the full skill body plus scaffolding. That +# expanded text is what flows into the agent loop — and into memory providers +# via MemoryManager. Providers that store or embed the raw user turn (mem0, +# openviking, hindsight, retaindb, byterover, honcho, supermemory) would +# otherwise capture the entire skill body instead of what the user actually +# asked. ``extract_user_instruction_from_skill_message`` recovers just the +# user's instruction so memory stays clean. +# +# These markers MUST stay byte-identical to the builders below +# (``_build_skill_message`` here, ``build_bundle_invocation_message`` in +# agent/skill_bundles.py). They are co-located with the single-skill builder +# on purpose, and the bundle markers are asserted against the bundle builder in +# tests/openviking_plugin/test_openviking.py::test_skill_markers_match_hermes_scaffolding. +# --------------------------------------------------------------------------- +_SKILL_INVOCATION_PREFIX = "[IMPORTANT: The user has invoked the " +_SINGLE_SKILL_MARKER = "The full skill content is loaded below.]" +_SINGLE_SKILL_INSTRUCTION = ( + "The user has provided the following instruction alongside the skill invocation: " +) +_RUNTIME_NOTE = "\n\n[Runtime note:" +_BUNDLE_MARKER = " skill bundle," +_BUNDLE_USER_INSTRUCTION = "\nUser instruction: " +_BUNDLE_FIRST_SKILL_BLOCK = "\n\n[Loaded as part of the " + + +def extract_user_instruction_from_skill_message(content: Any) -> Optional[str]: + """Recover the user's instruction from a slash-skill-expanded turn. + + Returns: + - The original string unchanged when it is NOT skill scaffolding + (a normal user message passes straight through). + - The extracted user instruction when the scaffolding carried one. + - ``None`` when the content is skill scaffolding with no user + instruction (i.e. a bare ``/skill`` invocation). Callers that feed + memory providers should skip the turn in that case — there is no + user content worth storing. + """ + if not isinstance(content, str): + return None + + if not content.startswith(_SKILL_INVOCATION_PREFIX): + return content + + if _BUNDLE_MARKER in content: + return _extract_bundle_user_instruction(content) + + if _SINGLE_SKILL_MARKER in content: + return _extract_single_skill_user_instruction(content) + + return None + + +def _extract_single_skill_user_instruction(message: str) -> Optional[str]: + # Single-skill format appends the user instruction after the skill body, so + # the last occurrence is the user-provided one; the body may quote this text. + marker_idx = message.rfind(_SINGLE_SKILL_INSTRUCTION) + if marker_idx < 0: + return None + + instruction = message[marker_idx + len(_SINGLE_SKILL_INSTRUCTION):] + runtime_idx = instruction.find(_RUNTIME_NOTE) + if runtime_idx >= 0: + instruction = instruction[:runtime_idx] + instruction = instruction.strip() + return instruction or None + + +def _extract_bundle_user_instruction(message: str) -> Optional[str]: + # Bundle format puts the user instruction before the loaded skills, so the + # first occurrence is the user-provided one. + marker_idx = message.find(_BUNDLE_USER_INSTRUCTION) + if marker_idx < 0: + return None + + instruction = message[marker_idx + len(_BUNDLE_USER_INSTRUCTION):] + first_skill_idx = instruction.find(_BUNDLE_FIRST_SKILL_BLOCK) + if first_skill_idx >= 0: + instruction = instruction[:first_skill_idx] + instruction = instruction.strip() + return instruction or None + def _resolve_skill_commands_platform() -> Optional[str]: """Return the current platform scope used for disabled-skill filtering. diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index 7f379220c50..3050eb9c43a 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -39,6 +39,7 @@ from urllib.parse import urlparse from urllib.request import url2pathname from agent.memory_provider import MemoryProvider +from agent.skill_commands import extract_user_instruction_from_skill_message from tools.registry import tool_error logger = logging.getLogger(__name__) @@ -66,60 +67,18 @@ _MEMORY_WRITE_TARGET_SUBDIR_MAP = { "memory": "patterns", } -_SKILL_INVOCATION_PREFIX = "[IMPORTANT: The user has invoked the " -_SINGLE_SKILL_MARKER = "The full skill content is loaded below.]" -_SINGLE_SKILL_INSTRUCTION = ( - "The user has provided the following instruction alongside the skill invocation: " -) -_BUNDLE_MARKER = " skill bundle," -_BUNDLE_USER_INSTRUCTION = "\nUser instruction: " -_BUNDLE_FIRST_SKILL_BLOCK = "\n\n[Loaded as part of the " -_RUNTIME_NOTE = "\n\n[Runtime note:" - def _derive_openviking_user_text(content: Any) -> str: - """Strip Hermes slash-skill scaffolding before sending content to OpenViking.""" - if not isinstance(content, str): - return "" + """Strip Hermes slash-skill scaffolding before sending content to OpenViking. - if not content.startswith(_SKILL_INVOCATION_PREFIX): - return content - - if _BUNDLE_MARKER in content: - return _extract_bundle_user_instruction(content) - - if _SINGLE_SKILL_MARKER in content: - return _extract_single_skill_user_instruction(content) - - return "" - - -def _extract_single_skill_user_instruction(message: str) -> str: - # Single-skill format appends the user instruction after the skill body, so - # the last occurrence is the user-provided one; the body may quote this text. - marker_idx = message.rfind(_SINGLE_SKILL_INSTRUCTION) - if marker_idx < 0: - return "" - - instruction = message[marker_idx + len(_SINGLE_SKILL_INSTRUCTION) :] - runtime_idx = instruction.find(_RUNTIME_NOTE) - if runtime_idx >= 0: - instruction = instruction[:runtime_idx] - return instruction.strip() - - -def _extract_bundle_user_instruction(message: str) -> str: - # Bundle format puts the user instruction before the loaded skills, so the - # first occurrence is the user-provided one. - marker_idx = message.find(_BUNDLE_USER_INSTRUCTION) - if marker_idx < 0: - return "" - - instruction = message[marker_idx + len(_BUNDLE_USER_INSTRUCTION) :] - first_skill_idx = instruction.find(_BUNDLE_FIRST_SKILL_BLOCK) - if first_skill_idx >= 0: - instruction = instruction[:first_skill_idx] - return instruction.strip() + Defense-in-depth: MemoryManager already strips skill scaffolding for the + whole provider fan-out (see ``MemoryManager._strip_skill_scaffolding``), so + in normal operation this receives already-clean text and passes it through + unchanged. It stays here so OpenViking is correct if its hooks are ever + invoked outside the manager. Delegates to the canonical extractor in + ``agent.skill_commands`` — no duplicated marker literals, no drift risk. + """ + return extract_user_instruction_from_skill_message(content) or "" # --------------------------------------------------------------------------- diff --git a/tests/agent/test_memory_skill_scaffolding.py b/tests/agent/test_memory_skill_scaffolding.py new file mode 100644 index 00000000000..3d26ba627b6 --- /dev/null +++ b/tests/agent/test_memory_skill_scaffolding.py @@ -0,0 +1,161 @@ +"""MemoryManager strips slash-skill scaffolding for every provider. + +When a user invokes a /skill or /bundle, Hermes expands the turn into a +model-facing message that embeds the full skill body. Feeding that verbatim to +memory providers pollutes their stores/embeddings with prompt scaffolding +instead of what the user actually asked. The strip lives once in MemoryManager +so it covers the whole provider fan-out — not per backend. + +See: agent.skill_commands.extract_user_instruction_from_skill_message and +MemoryManager._strip_skill_scaffolding. +""" + +from agent.memory_manager import MemoryManager +from agent.memory_provider import MemoryProvider +from agent.skill_commands import extract_user_instruction_from_skill_message + + +_SINGLE_SKILL_TURN = ( + '[IMPORTANT: The user has invoked the "skill-creator" skill, indicating they want ' + "you to follow its instructions. The full skill content is loaded below.]\n\n" + "# Skill Creator\n\n" + "Large skill body that must not be searched or embedded.\n\n" + "The user has provided the following instruction alongside the skill invocation: " + "make a skill for release triage" +) + +_BUNDLE_TURN = ( + '[IMPORTANT: The user has invoked the "backend-dev" skill bundle, ' + "loading 2 skills together. Treat every skill below as active guidance for this turn.]\n\n" + "Bundle: backend-dev\n" + "Skills loaded: test-driven-development, code-review\n\n" + "User instruction: fix the failing retrieval test\n\n" + '[Loaded as part of the "backend-dev" skill bundle.]\n\n' + "Large bundled skill body that must not be searched or embedded." +) + +_BARE_SKILL_TURN = ( + '[IMPORTANT: The user has invoked the "skill-creator" skill, indicating they want ' + "you to follow its instructions. The full skill content is loaded below.]\n\n" + "# Skill Creator\n\n" + "Large skill body, no user instruction." +) + + +class _RecordingProvider(MemoryProvider): + """Captures exactly what user text each fan-out method received.""" + + _name = "recording" + + def __init__(self): + self.prefetched = [] + self.queued = [] + self.synced = [] + + @property + def name(self) -> str: + return self._name + + def initialize(self, session_id: str = "", **kwargs) -> None: + pass + + def is_available(self) -> bool: + return True + + def system_prompt_block(self) -> str: + return "" + + def prefetch(self, query, *, session_id: str = "") -> str: + self.prefetched.append(query) + return "" + + def queue_prefetch(self, query, *, session_id: str = "") -> None: + self.queued.append(query) + + def sync_turn(self, user_content, assistant_content, *, session_id: str = "", messages=None) -> None: + self.synced.append(user_content) + + def get_tool_schemas(self): + return [] + + +def _manager_with_recorder(): + mgr = MemoryManager() + provider = _RecordingProvider() + mgr.add_provider(provider) + return mgr, provider + + +class TestExtractUserInstruction: + def test_non_string_returns_none(self): + assert extract_user_instruction_from_skill_message(None) is None + assert extract_user_instruction_from_skill_message(123) is None + assert extract_user_instruction_from_skill_message([{"text": "hi"}]) is None + + def test_plain_message_passes_through(self): + assert extract_user_instruction_from_skill_message("just a message") == "just a message" + + def test_single_skill_with_instruction(self): + assert ( + extract_user_instruction_from_skill_message(_SINGLE_SKILL_TURN) + == "make a skill for release triage" + ) + + def test_bundle_with_instruction(self): + assert ( + extract_user_instruction_from_skill_message(_BUNDLE_TURN) + == "fix the failing retrieval test" + ) + + def test_bare_skill_returns_none(self): + assert extract_user_instruction_from_skill_message(_BARE_SKILL_TURN) is None + + def test_runtime_note_trimmed_from_single_skill(self): + turn = _SINGLE_SKILL_TURN + "\n\n[Runtime note: in a subagent]" + assert ( + extract_user_instruction_from_skill_message(turn) + == "make a skill for release triage" + ) + + +class TestMemoryManagerStripsScaffolding: + def test_prefetch_all_strips_single_skill(self): + mgr, provider = _manager_with_recorder() + mgr.prefetch_all(_SINGLE_SKILL_TURN) + assert provider.prefetched == ["make a skill for release triage"] + + def test_prefetch_all_skips_bare_skill(self): + mgr, provider = _manager_with_recorder() + result = mgr.prefetch_all(_BARE_SKILL_TURN) + assert result == "" + assert provider.prefetched == [] + + def test_queue_prefetch_all_strips_bundle(self): + mgr, provider = _manager_with_recorder() + mgr.queue_prefetch_all(_BUNDLE_TURN) + mgr.flush_pending(timeout=5.0) + assert provider.queued == ["fix the failing retrieval test"] + + def test_queue_prefetch_all_skips_bare_skill(self): + mgr, provider = _manager_with_recorder() + mgr.queue_prefetch_all(_BARE_SKILL_TURN) + mgr.flush_pending(timeout=5.0) + assert provider.queued == [] + + def test_sync_all_strips_single_skill(self): + mgr, provider = _manager_with_recorder() + mgr.sync_all(_SINGLE_SKILL_TURN, "Done.") + mgr.flush_pending(timeout=5.0) + assert provider.synced == ["make a skill for release triage"] + + def test_sync_all_skips_bare_skill(self): + mgr, provider = _manager_with_recorder() + mgr.sync_all(_BARE_SKILL_TURN, "Done.") + mgr.flush_pending(timeout=5.0) + assert provider.synced == [] + + def test_plain_message_passes_through_unchanged(self): + mgr, provider = _manager_with_recorder() + mgr.sync_all("what's the weather", "Sunny.") + mgr.flush_pending(timeout=5.0) + assert provider.synced == ["what's the weather"] diff --git a/tests/openviking_plugin/test_openviking.py b/tests/openviking_plugin/test_openviking.py index ea95e386425..1c3365b9c02 100644 --- a/tests/openviking_plugin/test_openviking.py +++ b/tests/openviking_plugin/test_openviking.py @@ -107,10 +107,10 @@ class TestOpenVikingSkillQuerySafety: runtime_note="runtime detail", ) assert single is not None - assert openviking_plugin._SKILL_INVOCATION_PREFIX in single - assert openviking_plugin._SINGLE_SKILL_MARKER in single - assert openviking_plugin._SINGLE_SKILL_INSTRUCTION in single - assert openviking_plugin._RUNTIME_NOTE in single + assert skill_commands._SKILL_INVOCATION_PREFIX in single + assert skill_commands._SINGLE_SKILL_MARKER in single + assert skill_commands._SINGLE_SKILL_INSTRUCTION in single + assert skill_commands._RUNTIME_NOTE in single skill_bundles.scan_bundles() bundle_result = skill_bundles.build_bundle_invocation_message( @@ -119,9 +119,9 @@ class TestOpenVikingSkillQuerySafety: ) assert bundle_result is not None bundle, _, _ = bundle_result - assert openviking_plugin._BUNDLE_MARKER in bundle - assert openviking_plugin._BUNDLE_USER_INSTRUCTION in bundle - assert openviking_plugin._BUNDLE_FIRST_SKILL_BLOCK in bundle + assert skill_commands._BUNDLE_MARKER in bundle + assert skill_commands._BUNDLE_USER_INSTRUCTION in bundle + assert skill_commands._BUNDLE_FIRST_SKILL_BLOCK in bundle def test_queue_prefetch_searches_only_slash_skill_user_instruction(self, monkeypatch): RecordingVikingClient.calls = []