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.
2026-07-21 02:05:45 +00:00 · 2026-07-13 18:38:32 +08:00
parent bcee5a9061
commit 2fa0505070
3 changed files with 217 additions and 7 deletions
@@ -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)
@@ -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,
    }
 )

@@ -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]))