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/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index 8a394b4dc..aafb1c529 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -15,6 +15,7 @@ from typing import NamedTuple from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.sandbox.env_policy import build_sandbox_env from deerflow.sandbox.local.list_dir import list_dir +from deerflow.sandbox.path_patterns import build_output_mask_pattern from deerflow.sandbox.sandbox import Sandbox, _validate_extra_env from deerflow.sandbox.search import GrepMatch, find_glob_matches, find_grep_matches @@ -220,23 +221,22 @@ class LocalSandbox(Sandbox): @cached_property def _reverse_output_patterns(self) -> list[re.Pattern[str]]: """Compiled matchers for local paths in command output (longest local path first).""" - # Same segment-boundary lookahead as the forward patterns above, so a mount - # root does not match inside a sibling that merely shares its prefix - # (``.../skills`` inside ``.../skills-extra``). Without it the regex yields - # the bare root, which then *equals* the mount root and so satisfies - # ``_reverse_resolve_path``'s own ``+ "/"`` guard — the sibling is rewritten - # to a container path that forward resolution refuses to map back. + # The rule — segment boundary plus path tail — is owned by + # ``deerflow.sandbox.path_patterns`` and shared with + # ``sandbox.tools._compiled_mask_patterns``, the other site that rewrites host + # paths back to virtual ones. Its rationale (why the boundary class is + # text-oriented rather than shell-oriented like ``_command_pattern``, why ``$`` + # is load-bearing) lives with the owner rather than in a second copy here, which + # is what let the two drift before (#4035 added the boundary here and missed + # that site; #4053 added it there). # - # The boundary class mirrors ``_content_pattern``'s, not ``_command_pattern``'s: - # this runs over arbitrary command output, where a root can legitimately be - # followed by ``,`` ``:`` or ``\`` — all of which the shell-oriented class - # would reject. The trailing group keeps ``[/\\]`` so Windows paths still match. - # - # ``$`` is load-bearing: output ending exactly at a mount root would - # otherwise fail the lookahead and be emitted as the raw host path. - boundary = r"(?=/|$|[^\w./-])" - tail = r"(?:[/\\][^\s\"';&|<>()]*)?" - return [re.compile(re.escape(self._resolved_local_paths[m]) + boundary + tail) for m in self._mappings_by_local_specificity] + # What is specific to this site: without the boundary the regex yields the bare + # root, which then *equals* the mount root and so satisfies + # ``_reverse_resolve_path``'s own ``+ "/"`` guard — the sibling is rewritten to a + # container path that forward resolution refuses to map back. And bases stay + # separator-*sensitive*: they come from ``Path.resolve()`` and already carry the + # platform's separator, so relaxing them would widen what this masks. + return [build_output_mask_pattern(self._resolved_local_paths[m]) for m in self._mappings_by_local_specificity] @cached_property def _resolved_local_paths(self) -> dict[PathMapping, str]: diff --git a/backend/packages/harness/deerflow/sandbox/path_patterns.py b/backend/packages/harness/deerflow/sandbox/path_patterns.py new file mode 100644 index 000000000..2fa12077d --- /dev/null +++ b/backend/packages/harness/deerflow/sandbox/path_patterns.py @@ -0,0 +1,68 @@ +"""Shared construction of the host→virtual output-masking regexes. + +The boundary and tail are deliberately private: ``build_output_mask_pattern`` is +the only supported way to spell this rule, so a third site cannot import the +pieces and hand-roll a variant that drifts from the other two. + +Two independent call sites rewrite host paths back to their virtual form in +text that flows to the model: ``LocalSandbox._reverse_output_patterns`` (bash +output) and ``sandbox.tools._compiled_mask_patterns`` (glob/grep/ls results). +They must agree on where a host base is allowed to end, because both feed the +same downstream contract — a match that stops short of a real segment boundary +is rewritten to a container path that forward resolution then refuses to map +back. + +Keeping one copy of that rule per file is what let it drift: #4035 added the +segment boundary to the reverse patterns and missed the masking patterns, and +#4053 had to add the same boundary to the other copy. This module holds the +rule once so a third copy cannot silently disagree. + +The two sites are *not* identical, and the difference is deliberate — see +``separator_agnostic``. +""" + +from __future__ import annotations + +import re + +# Only match where a host base ends at a real path-segment boundary, so a mount +# root does not match inside a sibling that merely shares its prefix +# (``.../skills`` inside ``.../skills-extra``). +# +# The class is text-oriented, not shell-oriented (contrast +# ``LocalSandbox._command_pattern``): both callers run over arbitrary command +# output or file listings, where a root can legitimately be followed by ``,`` +# ``:`` or ``\``, all of which a shell-oriented class would reject. +# +# ``$`` is load-bearing: output ending exactly at a mount root would otherwise +# fail the lookahead and be emitted as the raw host path. +_SEGMENT_BOUNDARY = r"(?=/|$|[^\w./-])" + +# The path tail following the base. ``[/\\]`` keeps Windows-separated paths +# matching; the negated class stops at whitespace and shell punctuation so a +# path embedded in a larger line is not over-consumed. +_PATH_TAIL = r"(?:[/\\][^\s\"';&|<>()]*)?" + + +def build_output_mask_pattern(base: str, *, separator_agnostic: bool = False) -> re.Pattern[str]: + """Compile the matcher for one host ``base`` in model-visible output. + + Args: + base: Host path root to match (already resolved by the caller). + separator_agnostic: Accept either separator *inside* the base, so a + base captured with ``\\`` still matches output that spells the same + path with ``/``. ``sandbox.tools`` needs this because it derives its + bases from ``_path_variants`` (which yields Windows-style spellings) + and matches them against output whose separators it does not + control. ``LocalSandbox`` does not: its bases come from + ``Path.resolve()``, so they already carry the running platform's + separator, and relaxing them would widen what it masks. + + Returns: + A compiled pattern matching ``base`` at a segment boundary, plus an + optional path tail. + """ + escaped = re.escape(base) + if separator_agnostic: + escaped = escaped.replace(r"\\", r"[/\\]") + return re.compile(escaped + _SEGMENT_BOUNDARY + _PATH_TAIL) diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 9d44d6a43..780d57931 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -23,6 +23,7 @@ from deerflow.sandbox.exceptions import ( SandboxRuntimeError, ) from deerflow.sandbox.file_operation_lock import get_file_operation_lock +from deerflow.sandbox.path_patterns import build_output_mask_pattern from deerflow.sandbox.sandbox import Sandbox from deerflow.sandbox.sandbox_provider import get_sandbox_provider from deerflow.sandbox.search import GrepMatch @@ -727,20 +728,16 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple glob/grep match, so without this the same patterns are recompiled per match. """ - # Same segment-boundary lookahead as ``LocalSandbox._reverse_output_patterns`` - # (#4035), so a host base does not match inside a sibling that merely shares - # its prefix (``.../skills`` inside ``.../skills-extra``). Without it the - # regex yields the bare base, which then *equals* ``base`` in - # ``replace_match`` and so the sibling is rewritten to a container path that - # forward resolution refuses to map back. + # The segment boundary and path tail are shared with + # ``LocalSandbox._reverse_output_patterns`` — see + # ``deerflow.sandbox.path_patterns``, which owns that rule so the two copies + # cannot drift again (#4035 fixed one and missed the other; #4053 fixed the + # other). # - # The class mirrors ``_content_pattern``'s: this runs over arbitrary command - # output, where a base can legitimately be followed by ``,`` ``:`` or ``\``. - # ``$`` is load-bearing — output ending exactly at a base would otherwise - # fail the lookahead and be emitted as the raw host path. - boundary = r"(?=/|$|[^\w./-])" - tail = r"(?:[/\\][^\s\"';&|<>()]*)?" - + # ``separator_agnostic=True`` is the one thing this site does differently: + # its bases come from ``_path_variants``, which yields Windows-style + # spellings, and they are matched against output whose separators this layer + # does not control. compiled: list[tuple[re.Pattern[str], str, str]] = [] for host_base, virtual_base in sources: seen: set[str] = set() @@ -753,8 +750,7 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple if variant in seen: continue seen.add(variant) - escaped = re.escape(variant).replace(r"\\", r"[/\\]") - compiled.append((re.compile(escaped + boundary + tail), variant, virtual_base)) + compiled.append((build_output_mask_pattern(variant, separator_agnostic=True), variant, virtual_base)) return tuple(compiled) diff --git a/backend/tests/test_deferred_catalog.py b/backend/tests/test_deferred_catalog.py index 131f3f492..b4032c050 100644 --- a/backend/tests/test_deferred_catalog.py +++ b/backend/tests/test_deferred_catalog.py @@ -1,4 +1,5 @@ import pytest +from langchain_core.tools import StructuredTool from langchain_core.tools import tool as as_tool from deerflow.tools.builtins.tool_search import MAX_RESULTS, DeferredToolCatalog @@ -123,3 +124,13 @@ def test_hash_changes_with_membership(): c1 = DeferredToolCatalog((alpha_search, beta_translate)) c2 = DeferredToolCatalog((alpha_search,)) assert c1.hash != c2.hash + + +def test_search_select_not_capped_at_max_results(): + """select: must return ALL requested tools, even when >MAX_RESULTS.""" + tools = tuple(StructuredTool.from_function(func=lambda q="": q, name=f"tool_{i:02d}", description=f"Tool {i}") for i in range(8)) + cat = DeferredToolCatalog(tools) + names_csv = ",".join(f"tool_{i:02d}" for i in range(8)) + got = cat.search(f"select:{names_csv}") + assert len(got) == 8 + assert [t.name for t in got] == [f"tool_{i:02d}" for i in range(8)] diff --git a/backend/tests/test_sandbox_path_patterns.py b/backend/tests/test_sandbox_path_patterns.py new file mode 100644 index 000000000..46385bfec --- /dev/null +++ b/backend/tests/test_sandbox_path_patterns.py @@ -0,0 +1,129 @@ +"""Tests for the shared host→virtual output-mask pattern (``sandbox/path_patterns.py``). + +The rule these pin is not "the regex is correct" — that is #4035/#4053 — but +"there is exactly one copy of it, and extracting it did not change either call +site's matching". The two sites differ on one axis only (separator handling), +and that asymmetry is load-bearing: erasing it would widen ``LocalSandbox``'s +masking or narrow ``sandbox.tools``'s. + +The move itself was cleared by a differential against the *real* pre-extraction +expressions, run once on the parent commit. That run cannot be committed: after +this lands there is no old inline expression left to diff against, only the +frozen copies below. So the committed guard is the weaker snapshot, and its +red-ness rests on those literals — not on the length of ``_BASES``. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping +from deerflow.sandbox.path_patterns import build_output_mask_pattern +from deerflow.sandbox.tools import _compiled_mask_patterns + + +def _legacy_tools_pattern(base: str) -> re.Pattern[str]: + """The expression ``_compiled_mask_patterns`` inlined before the extraction.""" + escaped = re.escape(base).replace(r"\\", r"[/\\]") + return re.compile(escaped + r"(?=/|$|[^\w./-])" + r"(?:[/\\][^\s\"';&|<>()]*)?") + + +def _legacy_local_pattern(base: str) -> re.Pattern[str]: + """The expression ``_reverse_output_patterns`` inlined before the extraction.""" + return re.compile(re.escape(base) + r"(?=/|$|[^\w./-])" + r"(?:[/\\][^\s\"';&|<>()]*)?") + + +_BASES = [ + "/host/skills", + "/host/dir with spaces", + "/host/re+meta(chars)[x]", + "/host/dots.in.name", + "/Users/a/.deer-flow/users/u1/threads/t1/user-data", + "C:\\host\\skills", + "/host/技能", + # Drive root: the only base either caller can hand the helper that still ends in a + # separator (``Path.resolve()`` strips them everywhere else), so it is the one shape + # that goes red if the helper starts normalizing the base it is given. + "C:\\", +] + + +@pytest.mark.parametrize("base", _BASES) +def test_helper_reproduces_the_pre_extraction_expressions(base: str) -> None: + """Byte-identical to what each call site built inline, for both separator modes. + + This is the anchor for the move itself: edit the helper in a way that changes + either site's regex and this goes red. + """ + assert build_output_mask_pattern(base, separator_agnostic=True).pattern == _legacy_tools_pattern(base).pattern + assert build_output_mask_pattern(base).pattern == _legacy_local_pattern(base).pattern + + +def test_separator_agnostic_is_the_only_difference_between_the_two_modes() -> None: + """The asymmetry the helper must preserve rather than unify. + + ``sandbox.tools`` derives bases from ``_path_variants`` (Windows spellings) + and matches them against output whose separators it does not control, so a + ``\\``-spelled base must still match ``/``-spelled output. ``LocalSandbox`` + resolves its bases from the running platform and must not be widened. + """ + windows_base = "C:\\host\\skills" + posix_spelling = "C:/host/skills/file.md" + + assert build_output_mask_pattern(windows_base, separator_agnostic=True).search(posix_spelling) + assert build_output_mask_pattern(windows_base).search(posix_spelling) is None + + # On a base with no separator ambiguity the two modes agree exactly. + posix_base = "/host/skills" + assert build_output_mask_pattern(posix_base, separator_agnostic=True).pattern == build_output_mask_pattern(posix_base).pattern + + +def test_boundary_still_rejects_prefix_siblings_and_accepts_real_segments() -> None: + """The #4035/#4053 rule itself, now asserted once against the shared helper.""" + pattern = build_output_mask_pattern("/host/skills") + + # Matches: the root itself, a child, a Windows-separated child, and a root + # followed by text punctuation (``$`` and the ``[^\w./-]`` class). + assert pattern.fullmatch("/host/skills") + assert pattern.match("/host/skills/a/b.md") + assert pattern.match("/host/skills\\a\\b.md") + assert pattern.search("paths: /host/skills, and more") + + # Does not match inside a sibling that merely shares the prefix. + assert pattern.search("/host/skills-extra/file.md") is None + assert pattern.search("/host/skills.bak") is None + assert pattern.search("/host/skills2/file.md") is None + + +def test_local_sandbox_reverse_patterns_route_through_the_helper(tmp_path: Path) -> None: + """Call-site wiring: a re-inlined copy that *diverges* from the shared rule goes red. + + It does not (and cannot) catch a byte-identical re-inline — that is not yet a + defect. What it catches is the shape of the actual regression: #4035 changed + one copy of the rule and left the other behind. + """ + local = tmp_path / "skills" + local.mkdir() + sandbox = LocalSandbox( + id="local", + path_mappings=[PathMapping(container_path="/mnt/skills", local_path=str(local), read_only=True)], + ) + + resolved = str(Path(local).resolve()) + assert [p.pattern for p in sandbox._reverse_output_patterns] == [build_output_mask_pattern(resolved).pattern] + + +def test_tools_mask_patterns_route_through_the_helper(tmp_path: Path) -> None: + """Same wiring check for the other copy — and it must stay separator-agnostic.""" + host = tmp_path / "skills" + host.mkdir() + + compiled = _compiled_mask_patterns(((str(host), "/mnt/skills"),)) + + assert compiled + for pattern, variant, virtual_base in compiled: + assert virtual_base == "/mnt/skills" + assert pattern.pattern == build_output_mask_pattern(variant, separator_agnostic=True).pattern 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])) diff --git a/frontend/src/components/landing/hero.tsx b/frontend/src/components/landing/hero.tsx index 34aa91762..050e52db3 100644 --- a/frontend/src/components/landing/hero.tsx +++ b/frontend/src/components/landing/hero.tsx @@ -32,7 +32,7 @@ export function Hero({ className }: { className?: string }) { return (
@@ -47,7 +47,7 @@ export function Hero({ className }: { className?: string }) { />