From 2fa050507072675c6dd34fe17626a66003fc44fb Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Mon, 13 Jul 2026 03:38:32 -0700 Subject: [PATCH] fix(skills): activate a slash skill once per run, not per model call (#4103) * fix(skills): activate a slash skill once per run, not per model call SkillActivationMiddleware injects the activation reminder for a slash command via request.override(messages=...), which LangChain's create_agent uses for a single model call and never writes back to graph state. The dedup guard scans request.messages for a prior reminder, but model_node rebuilds request.messages fresh from persisted state on every tool-loop step, so the reminder is never present on the 2nd..Nth model call of a turn. Every model call therefore re-parsed the command, re-read SKILL.md from disk, re-injected the multi-KB body, and re-recorded an "activate" audit event, despite the code intending a single activation per run (#3861 semantics: one activation call, many follow-up model calls). Key the dedup off the run context instead, which LangGraph threads through every model-node call of a run (the same durable signal the request-scoped secret source already uses). The activation call records the slash message's identity in context; later calls for the same message skip re-activation. A new user slash message keys differently and still activates. Secret binding is unaffected: it already re-resolves from the persisted slash source on every call. Adds regression tests that rebuild the real multi-call turn state and assert a single activation across the tool loop, plus a test proving a new slash command still activates. * fix(skills): address review nits on run-scoped activation dedup - Extract _already_activated(run_context, run_key) so the dedup check mirrors the existing _has_existing_activation_for_target sibling instead of an inline dense conditional. - Compute _activation_run_key() once in _find_activation_target and thread it through _prepare_model_request instead of recomputing it at the write site, making the "same key for check and write" invariant explicit in the code rather than implicit. - Document why the run-context write is an overwrite rather than an append/set: only the latest real user message is ever considered an activation target, so there is nothing earlier in the run worth preserving. - Add a regression test locking in the degraded-path contract: when runtime.context is None, the middleware still activates per-call instead of crashing or wrongly no-op'ing. --- .../skill_activation_middleware.py | 77 +++++++++- .../deerflow/runtime/secret_context.py | 11 ++ backend/tests/test_slash_skills.py | 136 +++++++++++++++++- 3 files changed, 217 insertions(+), 7 deletions(-) diff --git a/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py index b3dea331f..73a26c1b8 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py @@ -20,6 +20,7 @@ from langchain_core.messages import AIMessage, HumanMessage from deerflow.runtime.secret_context import ( _SECRETS_BINDING_AUDIT_KEY, _SLASH_SECRET_SOURCE_KEY, + _SLASH_SKILL_ACTIVATION_RUN_KEY, ACTIVE_SECRETS_CONTEXT_KEY, extract_request_secrets, ) @@ -44,8 +45,15 @@ _SLASH_SKILL_ACTIVATION_TARGET_ID_KEY = "slash_skill_activation_target_id" # secrets — those are read from the live registry on each call, #3938). The # injection set is recomputed every model call, but a slash-activated skill must # stay bound for the rest of the run — the model's tool loop issues many model -# calls after the single activation call (#3861 semantics). Both live in -# secret_context so they are covered by REDACTED_CONTEXT_KEYS in one place. +# calls after the single activation call (#3861 semantics). +# _SLASH_SKILL_ACTIVATION_RUN_KEY: identity of the slash message already activated +# in this run, so the reminder injection + skill disk read + "activate" audit event +# fire once per user slash command instead of on every model call. The reminder is +# added via request.override(messages=...) for a single model call and never +# persisted to graph state, so the 2nd..Nth model call of a turn rebuilds +# request.messages from state without it — the run context is the only signal that +# survives the tool loop. All three live in secret_context so they are covered by +# REDACTED_CONTEXT_KEYS in one place. @dataclass(frozen=True, slots=True) @@ -207,7 +215,42 @@ Follow this skill before choosing a general workflow. Load supporting resources previous = messages[target_index - 1] return is_slash_skill_activation_reminder(previous) - def _find_activation_target(self, messages: list) -> tuple[int, HumanMessage, _ActivationResolution] | None: + @staticmethod + def _activation_run_key(target: HumanMessage) -> str: + """Stable identity for a user slash message, used to activate once per run. + + Prefers the message id (LangGraph assigns and preserves a stable id once a + message is in graph state); falls back to a digest of the genuine user text + so an id-less message still dedupes within a run. A new user slash message + (new id / new text) yields a new key, so it is not suppressed. + """ + if target.id: + return target.id + content = get_original_user_content_text(target.content, target.additional_kwargs) + return "sha256:" + hashlib.sha256(content.encode("utf-8")).hexdigest() + + @staticmethod + def _run_context(request: ModelRequest) -> dict | None: + runtime = getattr(request, "runtime", None) + context = getattr(runtime, "context", None) + return context if isinstance(context, dict) else None + + @staticmethod + def _already_activated(run_context: dict | None, run_key: str) -> bool: + """Whether ``run_key`` was already recorded as activated earlier in this run. + + Sibling to ``_has_existing_activation_for_target``: that helper catches an + activation reminder still present in the scanned ``messages`` window; this + one catches a prior activation recorded on ``run_context`` whose reminder + already fell out of that window (the tool-loop case — see + ``_SLASH_SKILL_ACTIVATION_RUN_KEY``). ``run_key`` is computed once by the + caller (``_find_activation_target``) and reused as-is at the write site in + ``_prepare_model_request``, so the same key is always used to check and to + record — this helper only ever checks membership, never computes the key. + """ + return isinstance(run_context, dict) and run_context.get(_SLASH_SKILL_ACTIVATION_RUN_KEY) == run_key + + def _find_activation_target(self, messages: list, *, run_context: dict | None = None) -> tuple[int, HumanMessage, _ActivationResolution, str] | None: if not messages: return None @@ -220,12 +263,22 @@ Follow this skill before choosing a general workflow. Load supporting resources return None if self._has_existing_activation_for_target(messages, target_index, target): return None + # This exact slash message may have already activated earlier in the run. + # The message scan above cannot catch it because the reminder lives only in + # a per-call request override, never in state — the run context is the + # durable signal (see _already_activated / _SLASH_SKILL_ACTIVATION_RUN_KEY). + # Skipping here avoids the redundant skill disk read, reminder re-injection, + # and duplicate "activate" audit. run_key is computed once here and threaded + # through to the write site in _prepare_model_request. + run_key = self._activation_run_key(target) + if self._already_activated(run_context, run_key): + return None content = get_original_user_content_text(target.content, target.additional_kwargs) resolution = self._resolve_activation(content) if resolution is None: return None - return target_index, target, resolution + return target_index, target, resolution, run_key @staticmethod def _record_activation(request: ModelRequest, activation: _Activation, *, hook: str) -> None: @@ -251,11 +304,12 @@ Follow this skill before choosing a general workflow. Load supporting resources logger.debug("Failed to record slash skill activation audit event", exc_info=True) def _prepare_model_request(self, request: ModelRequest, *, hook: str) -> tuple[ModelRequest | AIMessage | None, _Activation | None]: - target_and_resolution = self._find_activation_target(list(request.messages)) + run_context = self._run_context(request) + target_and_resolution = self._find_activation_target(list(request.messages), run_context=run_context) if target_and_resolution is None: return None, None - target_index, target, resolution = target_and_resolution + target_index, target, resolution, run_key = target_and_resolution if resolution.failure_message: return AIMessage(content=resolution.failure_message), None @@ -271,6 +325,17 @@ Follow this skill before choosing a general workflow. Load supporting resources activation.content_hash, ) self._record_activation(request, activation, hook=hook) + # Mark this slash message as activated for the run so the tool loop's later + # model calls skip the redundant re-activation (#3861: one activation call, + # many follow-up model calls). A new user slash message keys differently and + # still activates. Overwrite (`=`), not append/accumulate, is intentional: + # _find_activation_target only ever considers the latest real user message as + # an activation target, so there is nothing earlier in the run worth + # remembering once a new activation replaces it — do not "fix" this into a + # set. run_key is the same value already checked in _find_activation_target + # (computed once there, threaded through here) rather than recomputed. + if run_context is not None: + run_context[_SLASH_SKILL_ACTIVATION_RUN_KEY] = run_key activation_msg = self._make_activation_message(target, self._build_activation_reminder(activation)) messages = list(request.messages) messages.insert(target_index, activation_msg) diff --git a/backend/packages/harness/deerflow/runtime/secret_context.py b/backend/packages/harness/deerflow/runtime/secret_context.py index 8c3bc6c45..3c9aaee4c 100644 --- a/backend/packages/harness/deerflow/runtime/secret_context.py +++ b/backend/packages/harness/deerflow/runtime/secret_context.py @@ -59,6 +59,16 @@ def read_active_secrets(context: Any) -> dict[str, str]: _SLASH_SECRET_SOURCE_KEY = "__slash_skill_secret_source" _SECRETS_BINDING_AUDIT_KEY = "__skill_secrets_binding_audit" +# Identity of the latest slash activation that has already fired in this run, so +# the reminder injection, skill disk read, and ``activate`` audit event happen +# once per user slash command rather than on every model call of the tool loop. +# The reminder is injected into the per-call model request only and never written +# back to graph state, so a scan of ``request.messages`` cannot detect a prior +# activation on the 2nd..Nth model call — the run context is the only signal that +# survives (mirroring ``_SLASH_SECRET_SOURCE_KEY``). Holds a message id / content +# digest, never a secret value; listed below to keep the redaction guard complete. +_SLASH_SKILL_ACTIVATION_RUN_KEY = "__slash_skill_activation_run" + # Run-context keys whose values are request-scoped secrets and must be stripped # before a context mapping is serialized anywhere observable (traces, logs). REDACTED_CONTEXT_KEYS = frozenset( @@ -67,6 +77,7 @@ REDACTED_CONTEXT_KEYS = frozenset( ACTIVE_SECRETS_CONTEXT_KEY, _SLASH_SECRET_SOURCE_KEY, _SECRETS_BINDING_AUDIT_KEY, + _SLASH_SKILL_ACTIVATION_RUN_KEY, } ) diff --git a/backend/tests/test_slash_skills.py b/backend/tests/test_slash_skills.py index 3471707dd..5b04467db 100644 --- a/backend/tests/test_slash_skills.py +++ b/backend/tests/test_slash_skills.py @@ -4,7 +4,7 @@ from pathlib import Path from types import SimpleNamespace from langchain.agents.middleware.types import ModelRequest -from langchain_core.messages import AIMessage, HumanMessage +from langchain_core.messages import AIMessage, HumanMessage, ToolMessage from app.channels.commands import KNOWN_CHANNEL_COMMANDS from deerflow.agents.middlewares import skill_activation_middleware as middleware_module @@ -221,6 +221,140 @@ def test_skill_activation_middleware_dedupes_immediately_previous_activation_wit assert sum(is_slash_skill_activation_reminder(message) for message in captured["messages"]) == 1 +def test_skill_activation_middleware_activates_once_across_tool_loop(monkeypatch, tmp_path): + # Regression for the re-activation bug: within a single run the model node is + # invoked once per tool-loop step, each time rebuilding request.messages fresh + # from persisted graph state. The activation reminder is added via + # request.override(messages=...) for one model call only and is NEVER written + # back to state, so the 2nd model call's state is [user, ai(tool_call), tool] + # with no reminder to scan. Dedup must therefore key off the run context, which + # LangGraph threads through every model-node call of the run. + skill = _make_skill(tmp_path, "data-analysis", content="# Data Analysis\nUse pandas.") + disk_reads = {"n": 0} + real_read = SkillActivationMiddleware._read_skill_content + + def counting_read(skill_file, skills_root, *, storage=None): + disk_reads["n"] += 1 + return real_read(skill_file, skills_root, storage=storage) + + monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill])) + monkeypatch.setattr(SkillActivationMiddleware, "_read_skill_content", staticmethod(counting_read)) + + recorded = [] + journal = SimpleNamespace(record_middleware=lambda *args, **kwargs: recorded.append(kwargs)) + # One run context object, shared across every model call of the turn. + runtime = SimpleNamespace(context={"__run_journal": journal}) + middleware = SkillActivationMiddleware() + user = HumanMessage(content="/data-analysis analyze uploads/foo.csv", id="msg-1") + + # --- model call 1: the single activation call --- + first_capture = {} + + def first_handler(model_request: ModelRequest): + first_capture["messages"] = model_request.messages + return AIMessage(content="", tool_calls=[{"name": "echo", "args": {}, "id": "call-1"}], id="ai-1") + + first_result = middleware.wrap_model_call(_make_model_request([user], runtime=runtime), first_handler) + assert isinstance(first_result, AIMessage) + assert sum(is_slash_skill_activation_reminder(message) for message in first_capture["messages"]) == 1 + + # --- model call 2: same turn, real state after the tool result comes back. + # The reminder from call 1 is gone (never persisted), exactly as create_agent + # rebuilds it. It must NOT be re-injected. --- + ai_tool_call = first_result + tool_result = ToolMessage(content="echoed", tool_call_id="call-1", id="tool-1") + second_capture = {} + + def second_handler(model_request: ModelRequest): + second_capture["messages"] = model_request.messages + return AIMessage(content="final answer", id="ai-2") + + second_result = middleware.wrap_model_call(_make_model_request([user, ai_tool_call, tool_result], runtime=runtime), second_handler) + assert isinstance(second_result, AIMessage) + assert second_capture["messages"] == [user, ai_tool_call, tool_result] + assert sum(is_slash_skill_activation_reminder(message) for message in second_capture["messages"]) == 0 + + # Skill read from disk once and the activation audit event recorded once for the + # whole multi-call turn. + assert disk_reads["n"] == 1 + assert sum(1 for kwargs in recorded if kwargs.get("action") == "activate") == 1 + + +def test_skill_activation_middleware_reactivates_on_new_user_slash_command(monkeypatch, tmp_path): + # The run-scoped dedup must not suppress a genuinely new activation: a later + # user slash message (distinct id / text) keys differently, so it still + # activates even though an earlier slash message already activated in this run. + skill_a = _make_skill(tmp_path, "data-analysis", content="# Data Analysis\nUse pandas.") + skill_b = _make_skill(tmp_path, "frontend-design", content="# Frontend Design\nUse react.") + monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill_a, skill_b])) + + recorded = [] + journal = SimpleNamespace(record_middleware=lambda *args, **kwargs: recorded.append(kwargs)) + runtime = SimpleNamespace(context={"__run_journal": journal}) + middleware = SkillActivationMiddleware() + + first_msg = HumanMessage(content="/data-analysis analyze uploads/foo.csv", id="msg-1") + first_capture = {} + + def first_handler(model_request: ModelRequest): + first_capture["messages"] = model_request.messages + return AIMessage(content="done", id="ai-1") + + middleware.wrap_model_call(_make_model_request([first_msg], runtime=runtime), first_handler) + first_reminders = [message for message in first_capture["messages"] if is_slash_skill_activation_reminder(message)] + assert len(first_reminders) == 1 + assert "Use pandas." in first_reminders[0].content + + # New user turn in the same run context: a different slash command must activate. + second_msg = HumanMessage(content="/frontend-design build a form", id="msg-2") + second_capture = {} + + def second_handler(model_request: ModelRequest): + second_capture["messages"] = model_request.messages + return AIMessage(content="done", id="ai-2") + + middleware.wrap_model_call( + _make_model_request([first_msg, AIMessage(content="done", id="ai-1"), second_msg], runtime=runtime), + second_handler, + ) + second_reminders = [message for message in second_capture["messages"] if is_slash_skill_activation_reminder(message)] + assert len(second_reminders) == 1 + assert "Use react." in second_reminders[0].content + + # Two distinct activations recorded across the run. + assert sum(1 for kwargs in recorded if kwargs.get("action") == "activate") == 2 + + +def test_skill_activation_middleware_activates_per_call_when_run_context_is_none(monkeypatch, tmp_path): + # Regression for the degraded-path contract: when runtime.context is None + # (e.g. no run-scoped context is threaded through - the #3989 case), _run_context() + # normalizes it to None and the run-scoped dedup guard (_already_activated) must + # treat that as "nothing recorded yet" rather than crashing or wrongly skipping + # activation. The middleware should gracefully fall back to the original + # per-call activation behavior - no worse than before the run-context dedup + # was introduced. + skill = _make_skill(tmp_path, "data-analysis", content="# Data Analysis\nUse pandas.") + monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill])) + + middleware = SkillActivationMiddleware() + original = HumanMessage(content="/data-analysis analyze uploads/foo.csv", id="msg-1") + runtime = SimpleNamespace(context=None) + captured = {} + + def handler(model_request: ModelRequest): + captured["messages"] = model_request.messages + return AIMessage(content="ok") + + result = middleware.wrap_model_call(_make_model_request([original], runtime=runtime), handler) + + assert isinstance(result, AIMessage) + assert result.content == "ok" + activation_msg, user_msg = captured["messages"] + assert is_slash_skill_activation_reminder(activation_msg) + assert "Use pandas." in activation_msg.content + assert user_msg.content == original.content + + def test_skill_activation_middleware_async_injects_hidden_human_context_for_model_call(monkeypatch, tmp_path): skill = _make_skill(tmp_path, "data-analysis", content="# Data Analysis\nUse pandas.") monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill]))