mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-23 10:42:00 +00:00
fix(memory): strip skill scaffolding for all providers, not just openviking
Generalizes #32663 (@ehz0ah). The slash-skill scaffolding pollution affected every auto-syncing memory provider — mem0, hindsight, retaindb, byterover, honcho, supermemory all store/embed the raw user turn, so a /skill invocation poisoned their stores with the full skill body, not just openviking. - Lift the contributor's parser into agent/skill_commands.py as the canonical extract_user_instruction_from_skill_message(), co-located with the message builders so the markers can't drift. - Strip once in MemoryManager.{prefetch_all,queue_prefetch_all,sync_all} — fixes the whole provider fan-out, bare /skill turns are skipped entirely. - OpenViking's _derive_openviking_user_text() now delegates to the shared helper as defense-in-depth (no duplicated marker literals). - Marker-drift regression now asserts against the canonical skill_commands constants; add manager-level coverage proving every provider gets clean text.
This commit is contained in:
Teknium
parent
e3adbb5ae9
commit
c2c55c4443
5 changed files with 296 additions and 60 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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 ""
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
161
tests/agent/test_memory_skill_scaffolding.py
Normal file
161
tests/agent/test_memory_skill_scaffolding.py
Normal file
|
|
@ -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"]
|
||||
|
|
@ -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 = []
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue